From 3fcb89e4f01dc1b77ca1b8f2843f60c8f75b9ca8 Mon Sep 17 00:00:00 2001 From: Thomas Willson Date: Fri, 2 Oct 2026 15:56:20 -0700 Subject: [PATCH] Pick reviewers more fairly: own PRs don't count as load, ties go to the longest wait Two changes to the shared least-loaded pick, used by AssignReviewers and by ReviewSLA's out-of-office reassignment: - A PR doesn't count toward its author's load. Someone who assigns themselves to their own PRs looked busier than everyone else and was passed over. - PRs merge quickly, so most people sit at 0 open assigned PRs and most picks are ties. Ties now go to whoever was assigned to someone else's PR longest ago, and only then rotate by PR number, which doesn't remember who went recently. When each person was last assigned comes from a second, smaller query of recent assignment events; one combined query timed out. If it fails, ties fall back to PR number as before. Co-Authored-By: Claude Opus 5.5 --- AssignReviewers/README.md | 7 ++- AssignReviewers/assign.js | 6 ++- AssignReviewers/assign.test.js | 54 +++++++++++++++++++--- ReviewConfig/README.md | 2 +- ReviewConfig/pick.js | 84 ++++++++++++++++++++++++++++------ 5 files changed, 127 insertions(+), 26 deletions(-) diff --git a/AssignReviewers/README.md b/AssignReviewers/README.md index 62109cf..82b9461 100644 --- a/AssignReviewers/README.md +++ b/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. diff --git a/AssignReviewers/assign.js b/AssignReviewers/assign.js index 648906c..bfabb85 100644 --- a/AssignReviewers/assign.js +++ b/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()) { diff --git a/AssignReviewers/assign.test.js b/AssignReviewers/assign.test.js index a1499a9..345cdca 100644 --- a/AssignReviewers/assign.test.js +++ b/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({ diff --git a/ReviewConfig/README.md b/ReviewConfig/README.md index 1982e14..029d98e 100644 --- a/ReviewConfig/README.md +++ b/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 diff --git a/ReviewConfig/pick.js b/ReviewConfig/pick.js index b883871..f10b2f7 100644 --- a/ReviewConfig/pick.js +++ b/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 };