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 };