Pick reviewers more fairly: own PRs don't count as load, ties go to the longest wait

AssignReviewers/README.md

@@ -15,8 +15,11 @@ What it does each time a PR is opened ready or marked ready:
    at once, it waits up to two minutes for that PR's own run.
 3. **Picks the rest.** It adds the least-loaded domain approver if there's none, then the
    least-loaded rotation reviewer if there are fewer than two. Load is the number of open, ready PRs
-   already assigned to that person in the org; anyone the Rippling PTO calendar has out today or on
-   the next business day is skipped (Work From Home doesn't count), and ties rotate by PR number.
+   already assigned to that person in the org, not counting PRs they wrote themselves; anyone the
+   Rippling PTO calendar has out today or on the next business day is skipped (Work From Home
+   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.

AssignReviewers/assign.js

@@ -5,7 +5,8 @@
 // - Mid-stack, it copies the assignees of the nearest PR below it that has any, so the same two
 //   people own the whole stack.
 // - Otherwise it picks the least-loaded available member of each team: fewest open, ready PRs
-//   already assigned to them, skipping anyone the PTO calendar has out today or next business day.
+//   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.
 // - A PR that already has a domain approver and a second assignee only gets the review requests
@@ -93,7 +94,8 @@ module.exports = async ({ github, context, core, inputs, sleep = (ms) => new Pro
       if (candidates.length === 0) return null;
       away ??= await awaySoon({ url: inputs.ptoCalendarUrl, people: inputs.people, core, ...pto });
       const chosen = await pickLeastLoaded({ github, org: owner, core, candidates, away, seed: pr.number });
-      notes.push(`picked ${chosen.login} from ${team} (${chosen.load} open assigned PRs)`);
+      const last = chosen.lastAssigned ? `last assigned ${chosen.lastAssigned.slice(0, 10)}` : 'not assigned recently';
+      notes.push(`picked ${chosen.login} from ${team} (${chosen.load} open assigned PRs, ${last})`);
       return chosen.login;
     };
     if (!hasDomainApprover()) {

AssignReviewers/assign.test.js

@@ -6,17 +6,26 @@ const assign = require('./assign.js');
 const STAFF = ['staffA', 'staffB', 'staffC'];
 const EVERYONE = [...STAFF, 'devD', 'devE', 'devF'];
 
-// A fake GitHub: open PRs keyed by number, team membership and per-user load.
-function fakeGithub({ prs, load = {} }) {
+// A fake GitHub: open PRs keyed by number, team membership, per-user load, and when each person
+// was last assigned (`assigned`, login to ISO time).
+function fakeGithub({ prs, load = {}, assigned = {} }) {
   const calls = { assign: [], unassign: [], review: [], graphql: 0 };
   const byNumber = new Map(prs.map((p) => [p.number, { assignees: [], requested: [], teams: [], reviews: [], ...p }]));
   const teams = { embeddedreviewersstaff: STAFF, embeddedreviewers: EVERYONE };
   const user = (login) => ({ login, type: 'User' });
   const github = {
     paginate: async (fn, params) => (await fn(params)).data,
-    // Serves the load query from the fake PRs, as GitHub would.
-    graphql: async () => {
+    // Serves the load query from the fake PRs, as GitHub would, and the last-assigned query from
+    // `assigned`.
+    graphql: async (query) => {
       calls.graphql++;
+      if (query.includes('ASSIGNED_EVENT')) {
+        const nodes = Object.entries(assigned).map(([login, createdAt]) => ({
+          author: { login: 'someone' },
+          timelineItems: { nodes: [{ createdAt, assignee: { login } }] },
+        }));
+        return { organization: { repositories: { nodes: [{ pullRequests: { nodes } }] } } };
+      }
       const out = {
         organization: {
           repositories: {
@@ -26,11 +35,12 @@ function fakeGithub({ prs, load = {} }) {
                   nodes: [
                     ...[...byNumber.values()].map((p) => ({
                       isDraft: Boolean(p.draft),
+                      author: { login: p.author },
                       assignees: { nodes: p.assignees.map((login) => ({ login })) },
                     })),
                     // PRs in other repos, one per unit of preset load.
                     ...Object.entries(load).flatMap(([login, n]) =>
-                      Array.from({ length: n }, () => ({ isDraft: false, assignees: { nodes: [{ login }] } })),
+                      Array.from({ length: n }, () => ({ isDraft: false, author: { login: 'someone' }, assignees: { nodes: [{ login }] } })),
                     ),
                   ],
                 },
@@ -262,7 +272,27 @@ test('does not read the PTO calendar when the stack already has its assignees',
   assert.deepStrictEqual(fetches, []);
 });
 
-test('ties rotate by PR number rather than always going to the same person', async () => {
+test("a PR doesn't count toward its own author's load", async () => {
+  // staffA is assigned to three of their own open PRs; staffB and staffC each review one.
+  const own = [1, 2, 3].map((i) => ({ number: 130 + i, author: 'staffA', head: `own${i}`, base: 'main', assignees: ['staffA'] }));
+  const fake = fakeGithub({ prs: [...own, { number: 140, author: 'devE', head: 'a', base: 'main' }], load: { staffB: 1, staffC: 1 } });
+  await run(fake, 140);
+  assert.ok(fake.pr(140).assignees.includes('staffA'));
+});
+
+test('ties go to whoever was assigned longest ago, and anyone not assigned recently first', async () => {
+  const assigned = { staffA: '2026-10-01T20:00:00Z', staffB: '2026-09-29T20:00:00Z', staffC: '2026-09-30T20:00:00Z' };
+  const fake = fakeGithub({ prs: [{ number: 150, author: 'devE', head: 'a', base: 'main' }], assigned });
+  await run(fake, 150);
+  assert.ok(fake.pr(150).assignees.includes('staffB'));
+  // devD and devF haven't been assigned recently, so the rotation reviewer is one of them.
+  const recent = { ...assigned, devD: '2026-09-30T00:00:00Z' };
+  const second = fakeGithub({ prs: [{ number: 151, author: 'devE', head: 'a', base: 'main' }], assigned: recent });
+  await run(second, 151);
+  assert.ok(second.pr(151).assignees.includes('devF'));
+});
+
+test('with no recent assignments, ties rotate by PR number rather than always going to the same person', async () => {
   const picks = new Set();
   for (const number of [60, 61, 62]) {
     const fake = fakeGithub({ prs: [{ number, author: 'devE', head: 'a', base: 'main' }] });
@@ -330,6 +360,18 @@ test('still assigns when the load query fails', async () => {
   assert.ok(logs.some((l) => l.startsWith('WARN')));
 });
 
+test('if only the last-assigned query fails, it still picks by load, and warns', async () => {
+  const fake = fakeGithub({ prs: [{ number: 160, author: 'devE', head: 'a', base: 'main' }], load: { staffA: 1, staffC: 1 } });
+  const graphql = fake.github.graphql;
+  fake.github.graphql = async (query, vars) => {
+    if (query.includes('ASSIGNED_EVENT')) throw new Error('HTTP 502');
+    return graphql(query, vars);
+  };
+  const logs = await run(fake, 160);
+  assert.ok(fake.pr(160).assignees.includes('staffB'));
+  assert.ok(logs.includes("WARN Couldn't read when reviewers were last assigned (HTTP 502); breaking ties by PR number."));
+});
+
 test('counts assignments made moments ago by other runs', async () => {
   // Two PRs opened back to back: the second must see the first one's fresh assignment.
   const fake = fakeGithub({

ReviewConfig/README.md

@@ -27,7 +27,7 @@ Whoever adds or removes someone from the review teams updates it at the same tim
 | `holidays.json` | Company paid holidays, by year | By hand, each year, from the holiday calendar People Operations publishes. Add next year's before Jan 1 |
 | `pto.js` | Reads the Rippling PTO calendar feed | — |
 | `hours.js` | Business hours: how many lie between two times, and when a target falls due | — |
-| `pick.js` | The least-loaded pick of a reviewer from a team | — |
+| `pick.js` | The least-loaded pick of a reviewer from a team, ties going to whoever was assigned longest ago | — |
 
 ## Who is out
 

ReviewConfig/pick.js

@@ -6,22 +6,33 @@ async function teamMembers(github, org, team) {
   return members.map((m) => m.login);
 }
 
-// For each login: how many open, ready PRs in the org already have them as assignee.
-async function reviewerLoad(github, org, logins, core) {
-  if (logins.length === 0) return {};
+// For each login: `load`, how many open, ready PRs in the org have them as assignee, and
+// `lastAssigned`, when they were last assigned to a PR (an ISO time, or absent if not recently).
+// A PR doesn't count for its own author, so assigning yourself to your own PR doesn't make you
+// look busy.
+async function reviewerHistory(github, org, logins, core) {
+  if (logins.length === 0) return { load: {}, lastAssigned: {} };
+  let load;
   try {
-    return await queryLoad(github, org, logins);
+    load = await queryLoad(github, org, logins);
   } catch (error) {
     core.warning(`Couldn't read reviewer load (${error.message}); picking without it.`);
-    return Object.fromEntries(logins.map((login) => [login, 0]));
+    load = Object.fromEntries(logins.map((login) => [login, 0]));
   }
+  let lastAssigned = {};
+  try {
+    lastAssigned = await queryLastAssigned(github, org, logins);
+  } catch (error) {
+    core.warning(`Couldn't read when reviewers were last assigned (${error.message}); breaking ties by PR number.`);
+  }
+  return { load, lastAssigned };
 }
 
-// Load is counted from live PR data rather than GitHub search, whose index lags new assignments
-// by minutes: several PRs opened close together would otherwise all see stale counts. The query
-// covers the org's most recently pushed repositories, which is where open PRs live.
+// Both are read from live PR data rather than GitHub search, whose index lags new assignments by
+// minutes: several PRs opened close together would otherwise all see stale counts. They cover the
+// org's most recently pushed repositories, which is where open PRs live.
 async function queryLoad(github, org, logins) {
-  const prFields = 'pageInfo { hasNextPage endCursor } nodes { isDraft assignees(first: 10) { nodes { login } } }';
+  const prFields = 'pageInfo { hasNextPage endCursor } nodes { isDraft author { login } assignees(first: 10) { nodes { login } } }';
   const data = await github.graphql(
     `query($org: String!) {
       organization(login: $org) {
@@ -37,7 +48,7 @@ async function queryLoad(github, org, logins) {
     for (const pullRequest of pullRequests?.nodes ?? []) {
       if (pullRequest.isDraft) continue;
       for (const assignee of pullRequest.assignees?.nodes ?? []) {
-        if (assignee.login in load) load[assignee.login]++;
+        if (assignee.login in load && assignee.login !== pullRequest.author?.login) load[assignee.login]++;
       }
     }
   };
@@ -59,18 +70,61 @@ async function queryLoad(github, org, logins) {
   return load;
 }
 
+// When each login was last assigned to someone else's PR, from the assignment events on the most
+// recently updated PRs of the most recently pushed repositories. Kept small: one larger query
+// times out.
+async function queryLastAssigned(github, org, logins) {
+  const data = await github.graphql(
+    `query($org: String!) {
+      organization(login: $org) {
+        repositories(first: 20, orderBy: { field: PUSHED_AT, direction: DESC }) {
+          nodes {
+            pullRequests(first: 30, orderBy: { field: UPDATED_AT, direction: DESC }) {
+              nodes {
+                author { login }
+                timelineItems(last: 10, itemTypes: [ASSIGNED_EVENT]) {
+                  nodes { ... on AssignedEvent { createdAt assignee { ... on User { login } } } }
+                }
+              }
+            }
+          }
+        }
+      }
+    }`,
+    { org },
+  );
+  const wanted = new Set(logins);
+  const lastAssigned = {};
+  for (const repository of data.organization?.repositories?.nodes ?? []) {
+    for (const pullRequest of repository.pullRequests?.nodes ?? []) {
+      for (const event of pullRequest.timelineItems?.nodes ?? []) {
+        const login = event.assignee?.login;
+        if (!wanted.has(login) || login === pullRequest.author?.login || !event.createdAt) continue;
+        if (!lastAssigned[login] || event.createdAt > lastAssigned[login]) lastAssigned[login] = event.createdAt;
+      }
+    }
+  }
+  return lastAssigned;
+}
+
 // The least-loaded candidate who isn't away, or the least-loaded of everyone if they all are.
-// Ties rotate by `seed` (the PR number) so they don't always go to the same person.
-// Returns { login, load } or null when there are no candidates.
+// Most people have no open assigned PRs most of the time, since PRs merge quickly, so ties are
+// common: they go to whoever was assigned longest ago (anyone not assigned recently first), and
+// only then rotate by `seed` (the PR number).
+// Returns { login, load, lastAssigned } or null when there are no candidates.
 async function pickLeastLoaded({ github, org, core, candidates, away, seed }) {
   if (candidates.length === 0) return null;
   const sorted = [...candidates].sort();
-  const load = await reviewerLoad(github, org, sorted, core);
+  const { load, lastAssigned } = await reviewerHistory(github, org, sorted, core);
   const available = sorted.filter((login) => !away.has(login));
   const pool = available.length > 0 ? available : sorted;
   const least = Math.min(...pool.map((login) => load[login]));
   const tied = pool.filter((login) => load[login] === least);
-  return { login: tied[seed % tied.length], load: least };
+  const since = (login) => (lastAssigned[login] ? Date.parse(lastAssigned[login]) : -Infinity);
+  const longest = Math.min(...tied.map(since));
+  const waited = tied.filter((login) => since(login) === longest);
+  const login = waited[seed % waited.length];
+  return { login, load: least, lastAssigned: lastAssigned[login] ?? null };
 }
 
-module.exports = { pickLeastLoaded, reviewerLoad, teamMembers };
+module.exports = { pickLeastLoaded, reviewerHistory, teamMembers };