CI: fix buildBody sign-off and add non-CODEOWNER reviewer tests (#6461)

* CI: fix buildBody to accept any owner approval, not only the assigned reviewer

PR #6446 narrowed the approver lookup to effectiveReviewers (the confirmed-
requested subset), inadvertently breaking the case where a non-assigned owner
approves. Restore the original intent: any CODEOWNER's approval signs off an
area; fall back to effectiveReviewers only when no CODEOWNER is assigned.

* text updates

* CI: add tests for non-CODEOWNER reviewer sign-off path (#6446)

Three tests cover the nonOwnerReviewers fallback introduced in #6446:
- pending mention when a non-owner is manually assigned
- non-owner approval signs off an area with no CODEOWNER assigned
- non-owner approval does NOT sign off when a CODEOWNER was assigned

* CI: restore review_requested/review_request_removed triggers

These were dropped in #6453 when ready_for_review was added, breaking
automatic checklist updates on manual reviewer changes.

Both events fall through to the preserve-existing path in
coordinateReviewers when a checklist already exists, so the checklist
body is simply rebuilt with the current requested_reviewers set.

Also extend the opening-race detection (previously called isFirstSyncRace)
to cover review_requested: for any PR, CODEOWNERS auto-assignment fires
review_requested shortly after opened, and with cancel-in-progress: true
that run can cancel the opened run. If no checklist exists yet, treat the
review_requested event as a new-PR event and run full selection.

* CI: show only one reviewer per checklist area

Each area's pending mention now shows the primary (first) effective reviewer
rather than all confirmed-requested owners joined with commas.

* CI: revert to showing all confirmed reviewers per checklist area

A manually added person who is also a CODEOWNER for that area should be
shown alongside the load-balanced pick, not hidden. "One reviewer per area"
only describes chooseReviewers' default selection, not a display constraint.

.github/scripts/review-checklist.js

@@ -167,11 +167,16 @@ function buildBody(touchedAreas, approvedUsers, confirmedRequested) {
     // If no CODEOWNER is assigned for this area, fall back to non-CODEOWNER
     // reviewers so manually-assigned people are shown and their approval counts.
     const effectiveReviewers = ownerReviewers.length > 0 ? ownerReviewers : nonOwnerReviewers;
-    const approver  = effectiveReviewers.find(o => approvedUsers.has(o));
+    // Any owner's approval counts for sign-off, not only the assigned reviewer's.
+    // Fall back to effectiveReviewers for areas with no CODEOWNER (non-owner assignee).
+    const approver  = area.owners.find(o => approvedUsers.has(o))
+      || effectiveReviewers.find(o => approvedUsers.has(o));
     const signedOff = !!approver;
     const box       = signedOff ? 'x' : ' ';
     const tick      = signedOff ? ' ✅' : '';
-    // Signed off: show who approved. Pending: show all effective reviewers.
+    // Signed off: show who approved. Pending: show all confirmed reviewers for
+    // this area (normally just the load-balanced pick, plus any CODEOWNER who
+    // was manually added on top of it).
     const mention   = approver
       ? ` — @${approver}`
       : effectiveReviewers.length > 0 ? ` — ${effectiveReviewers.map(o => `@${o}`).join(', ')}` : '';
@@ -185,9 +190,8 @@ function buildBody(touchedAreas, approvedUsers, confirmedRequested) {
     MARKER,
     '## Review Checklist',
     '',
-    'This PR touches the following areas. Each needs at least one',
-    'sign-off from its listed owners before merging — an approval',
-    'covering only one area does **not** satisfy the others.',
+    'This PR touches the following areas. Each needs a sign-off',
+    'from its listed owners before merging.',
     '',
     ...rows,
   ];
@@ -265,8 +269,9 @@ async function removeUnselectedAfterDelay(github, core, { owner, repo, pr_number
 //   synchronize (normal push to open PR, checklist already exists)
 //               → preserve the existing assignment unchanged
 //
-//   new-PR      (opened / reopened / ready_for_review, or first-synchronize
-//                race where opened was cancelled before the checklist existed)
+//   new-PR      (opened / reopened / ready_for_review, or opening-race where
+//                opened was cancelled by synchronize or review_requested before
+//                the checklist existed)
 //     draft     → clear auto-assignments; defer requests until ready for review
 //     non-draft → full flow: clear → request → wait 15 s → re-clear
 //
@@ -301,21 +306,22 @@ async function coordinateReviewers(github, context, core, {
     core.info(`Author ${prAuthor} is not a code owner — skipping assignee`);
   }
 
-  // ── synchronize race detection ───────────────────────────────────────────
-  // When a fork PR is created, GitHub fires both `opened` and `synchronize`.
-  // With cancel-in-progress: true the synchronize run can win and the opened
-  // cleanup is cancelled — leaving all CODEOWNERS auto-assignments intact.
+  // ── opening race detection ───────────────────────────────────────────────
+  // For fork PRs, GitHub fires opened + synchronize in quick succession; for
+  // any PR, CODEOWNERS auto-assignment fires review_requested shortly after
+  // opened. With cancel-in-progress: true the last event's run wins, and the
+  // opened cleanup is cancelled — leaving auto-assignments intact.
   // Detect this by checking whether a checklist comment exists yet; if not,
-  // treat this synchronize as a new-PR event.
-  const isFirstSyncRace = action === 'synchronize' &&
-                          !(await checklistExists(github, pr));
+  // treat the event as a new-PR event and run full selection.
+  const isOpeningRace = (action === 'synchronize' || action === 'review_requested') &&
+                        !(await checklistExists(github, pr));
 
   const isNewPR = ['opened', 'reopened', 'ready_for_review'].includes(action) ||
-                  isFirstSyncRace;
+                  isOpeningRace;
 
-  // ── synchronize (normal) ─────────────────────────────────────────────────
+  // ── preserve-existing (synchronize / reviewer change) ────────────────────
   if (!isNewPR) {
-    core.info('synchronize — preserving existing reviewer assignments');
+    core.info(`${action} — preserving existing reviewer assignments`);
     return new Set(existingRequested);
   }
 

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

@@ -399,6 +399,34 @@ test('buildBody: always contains the marker', () => {
   assert.ok(body.includes('<!-- hdf5-review-checklist-v1 -->'));
 });
 
+// Non-CODEOWNER reviewer fallback (added by #6446): when no owner of the area
+// is in confirmedRequested, a manually-assigned non-owner reviewer is shown and
+// their approval counts as sign-off.
+
+test('buildBody: non-owner reviewer shown as pending when no area owner is assigned', () => {
+  // alice owns /src/ but was not requested; charlie (not an owner) was manually assigned
+  const areas = [makeArea('src', ['alice'], 10)];
+  const body  = buildBody(areas, new Set(), new Set(['charlie']));
+  assert.ok(body.includes('- [ ] **src**'));
+  assert.ok(body.includes('— @charlie'));
+  assert.ok(!body.includes('@alice'));
+});
+
+test('buildBody: non-owner reviewer approval signs off area when no CODEOWNER is assigned', () => {
+  const areas = [makeArea('src', ['alice'], 10)];
+  const body  = buildBody(areas, new Set(['charlie']), new Set(['charlie']));
+  assert.ok(body.includes('- [x] **src** ✅'));
+  assert.ok(body.includes('— @charlie'));
+});
+
+test('buildBody: non-owner reviewer approval does NOT sign off when a CODEOWNER was assigned', () => {
+  // alice (owner) was assigned; charlie (non-owner) also approves — alice's sign-off is still required
+  const areas = [makeArea('src', ['alice'], 10)];
+  const body  = buildBody(areas, new Set(['charlie']), new Set(['alice', 'charlie']));
+  assert.ok(body.includes('- [ ] **src**'));
+  assert.ok(!body.includes('✅'));
+});
+
 // ----------------------------------------------------------------
 // Summary
 // ----------------------------------------------------------------

.github/workflows/review-checklist.yml

@@ -23,7 +23,7 @@ on:
   # Safe: checkout has no ref: override so the base branch (develop) is always
   # used — the fork's code is never checked out or executed.
   pull_request_target:
-    types: [opened, synchronize, reopened, ready_for_review]
+    types: [opened, synchronize, reopened, ready_for_review, review_requested, review_request_removed]
     branches: [develop]
   workflow_run:
     workflows: ["Review Checklist (gather)"]