AssignReviewers: request each review team member individually

AssignReviewers/README.md

@@ -20,9 +20,12 @@ What it does each time a PR is opened ready or marked ready:
    doesn't count). PRs merge quickly, so most people are at 0 most of the time and ties are common:
    they go to whoever was assigned to someone else's PR longest ago (anyone not assigned recently
    first), then rotate by PR number.
-4. **Requests reviewers.** It requests the review team (default `embeddedreviewers`) unless someone
-   from it has already been requested or has reviewed, so restacks don't re-request approvers. An
-   assignee from outside the team is requested individually.
+4. **Requests reviewers.** It requests each member of the review team (default `embeddedreviewers`)
+   individually, plus any assignee from outside the team. It leaves out the author, anyone already
+   requested, and anyone whose approval stands, so reruns and draft round-trips don't re-request
+   approvers, while a comment left early doesn't keep the rest of the team from being asked. The
+   team itself isn't requested: its auto-assignment would turn that back into requests for
+   everyone, approvers included.
 
 The author is never an assignee; a self-assignment is removed. Drafts, bot PRs and Graphite
 merge-queue PRs are skipped. Team review auto-assignment settings are left as they are.

AssignReviewers/action.yml

@@ -16,7 +16,9 @@ inputs:
     description: Team the domain approver is picked from.
     default: embeddedreviewersstaff
   review-team:
-    description: Team requested as reviewers on every ready PR. Defaults to the rotation team.
+    description: >
+      Team whose members are each requested as reviewers on every ready PR, except the author,
+      anyone already requested and anyone whose approval stands. Defaults to the rotation team.
     default: ''
   max-stack-depth:
     description: How many PRs down the stack to look for assignees to inherit.

AssignReviewers/assign.js

@@ -7,8 +7,9 @@
 // - Otherwise it picks the least-loaded available member of each team: fewest open, ready PRs
 //   already assigned to them (not counting their own), skipping anyone the PTO calendar has out
 //   today or next business day. Ties go to whoever was assigned longest ago.
-// - The author is never an assignee (a self-assignment is removed). The review team is requested
-//   unless someone from it already is, and an assignee outside the team is requested individually.
+// - The author is never an assignee (a self-assignment is removed). Each member of the review team,
+//   and any assignee outside it, is requested individually, unless they already are or their
+//   approval stands.
 // - A PR that already has a domain approver and a second assignee only gets the review requests
 //   topped up, so reruns are harmless and hand-picked assignees are kept.
 
@@ -127,24 +128,29 @@ module.exports = async ({ github, context, core, inputs, sleep = (ms) => new Pro
     await github.rest.issues.removeAssignees({ owner, repo, issue_number: pr.number, assignees: [author] });
     notes.push(`removed author ${author} from assignees`);
   }
-  const reviewed = new Set(
-    humanLogins((await github.paginate(github.rest.pulls.listReviews, { owner, repo, pull_number: pr.number, per_page: 100 })).map((r) => r.user)),
-  );
+  // Everyone on the review team, and any assignee from outside it (a designated domain owner, say),
+  // is requested individually rather than as a team: the team's auto-assignment turns a team
+  // request back into the same individual requests, approvers included, so the only way to leave
+  // out the people who don't need asking is per person. That's the author, anyone already
+  // requested, and anyone whose approval stands. A review that's only a comment, or an approval
+  // since dismissed or replaced by a change request, doesn't count, so an early comment doesn't
+  // keep anyone else from being asked.
+  const reviews = await github.paginate(github.rest.pulls.listReviews, { owner, repo, pull_number: pr.number, per_page: 100 });
+  const decisive = new Map();
+  for (const review of reviews) {
+    if (!isBot(review.user) && (review.state === 'APPROVED' || review.state === 'CHANGES_REQUESTED')) decisive.set(review.user.login, review.state);
+  }
+  const approved = (login) => decisive.get(login) === 'APPROVED';
   const requested = new Set(humanLogins(current.requested_reviewers));
-  const involved = (login) => requested.has(login) || reviewed.has(login);
-  // The whole review team is requested, as authors used to do by hand, unless someone from it
-  // already has been or has reviewed (so reruns and restacks don't re-request approvers).
+  // A team that's already requested (by hand, say) already covers its members.
   const reviewTeamMembers = await teamMembers(github, owner, reviewTeam);
-  const teamAlreadyRequested = (current.requested_teams || []).some((t) => t.slug === reviewTeam);
-  if (!teamAlreadyRequested && !reviewTeamMembers.some((login) => login !== author && involved(login))) {
-    await github.rest.pulls.requestReviewers({ owner, repo, pull_number: pr.number, team_reviewers: [reviewTeam] });
-    for (const login of reviewTeamMembers) requested.add(login);
-    notes.push(`requested team ${reviewTeam}`);
-  }
-  // Assignees from outside the team (a designated domain owner, say) are requested individually.
-  const toRequest = [...named].filter((login) => !involved(login));
+  if ((current.requested_teams || []).some((t) => t.slug === reviewTeam)) for (const login of reviewTeamMembers) requested.add(login);
+  const toRequest = [...new Set([...reviewTeamMembers, ...named])].filter(
+    (login) => login !== author && !requested.has(login) && !approved(login),
+  );
   if (toRequest.length > 0) {
     await github.rest.pulls.requestReviewers({ owner, repo, pull_number: pr.number, reviewers: toRequest });
+    notes.push(`requested ${toRequest.join(', ')}`);
   }
 
   core.info(`Assignees: ${[...named].join(', ') || 'none'}${notes.length ? ` (${notes.join('; ')})` : ''}.`);

AssignReviewers/assign.test.js

@@ -165,7 +165,10 @@ test('bottom of stack: least-loaded staff approver and rotation reviewer, whole
   });
   await run(fake, 10);
   assert.deepStrictEqual(fake.pr(10).assignees, ['staffB', 'devF']);
-  assert.deepStrictEqual(fake.pr(10).teams, ['embeddedreviewers']);
+  // Each member but the author, individually, in one request; never the team itself.
+  assert.deepStrictEqual(fake.pr(10).requested, ['staffA', 'staffB', 'staffC', 'devD', 'devF']);
+  assert.deepStrictEqual(fake.pr(10).teams, []);
+  assert.strictEqual(fake.calls.review.length, 1);
 });
 
 test('staff author is never an assignee', async () => {
@@ -342,7 +345,7 @@ test("replaces an author's self-assignment", async () => {
   assert.strictEqual(fake.pr(70).assignees.length, 2);
 });
 
-test('keeps hand-picked assignees and does not re-request a team someone already reviewed for', async () => {
+test('keeps hand-picked assignees and does not re-request someone whose approval stands', async () => {
   const fake = fakeGithub({
     prs: [
       {
@@ -351,15 +354,39 @@ test('keeps hand-picked assignees and does not re-request a team someone already
         head: 'a',
         base: 'main',
         assignees: ['staffB', 'devF'],
+        requested: ['devD'],
         reviews: [['staffB', 'APPROVED']],
       },
     ],
   });
   await run(fake, 71);
   assert.deepStrictEqual(fake.calls.assign, []);
-  assert.ok(fake.calls.review.every((c) => c.team_reviewers.length === 0));
-  // devF hasn't reviewed or been requested, so they're requested individually.
-  assert.deepStrictEqual(fake.pr(71).requested, ['devF']);
+  // staffB approved and devD is already requested; everyone else on the team is asked.
+  assert.deepStrictEqual(fake.calls.review, [{ pull_number: 71, reviewers: ['staffA', 'staffC', 'devF'], team_reviewers: [] }]);
+});
+
+test('an early comment doesn\'t keep the rest of the team from being asked, and neither does a superseded approval', async () => {
+  // staffA commented while it was a draft; devD approved and then requested changes.
+  const fake = fakeGithub({
+    prs: [{ number: 74, author: 'devE', head: 'a', base: 'main', assignees: ['staffA', 'devD'], reviews: [['staffA', 'COMMENTED'], ['devD', 'APPROVED'], ['devD', 'CHANGES_REQUESTED']] }],
+  });
+  await run(fake, 74);
+  assert.deepStrictEqual(fake.pr(74).requested, ['staffA', 'staffB', 'staffC', 'devD', 'devF']);
+});
+
+test('a rerun after everyone was asked requests no one', async () => {
+  const fake = fakeGithub({ prs: [{ number: 75, author: 'devE', head: 'a', base: 'main' }] });
+  await run(fake, 75);
+  await run(fake, 75);
+  assert.strictEqual(fake.calls.review.length, 1);
+});
+
+test('when the team itself is already requested, its members aren\'t asked again individually', async () => {
+  const fake = fakeGithub({
+    prs: [{ number: 76, author: 'devE', head: 'a', base: 'main', assignees: ['staffA', 'ownerX'], teams: ['embeddedreviewers'] }],
+  });
+  await run(fake, 76);
+  assert.deepStrictEqual(fake.pr(76).requested, ['ownerX']);
 });
 
 test('requests an assignee from outside the team individually', async () => {
@@ -370,8 +397,8 @@ test('requests an assignee from outside the team individually', async () => {
     ],
   });
   await run(fake, 73);
-  assert.deepStrictEqual(fake.pr(73).teams, ['embeddedreviewers']);
-  assert.deepStrictEqual(fake.pr(73).requested, ['ownerX']);
+  assert.deepStrictEqual(fake.pr(73).teams, []);
+  assert.deepStrictEqual(fake.pr(73).requested, ['staffA', 'staffB', 'staffC', 'devD', 'devF', 'ownerX']);
 });
 
 test('ignores drafts, bots and merge-queue PRs', async () => {