Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 3 additions & 3 deletions .github/workflows/assign-reviewers-tests.yml
Original file line number Diff line number Diff line change
Expand Up @@ -2,10 +2,10 @@ name: AssignReviewers tests

on:
pull_request:
paths: [AssignReviewers/**]
paths: [AssignReviewers/**, ReviewConfig/**]
push:
branches: [main]
paths: [AssignReviewers/**]
paths: [AssignReviewers/**, ReviewConfig/**]

permissions:
contents: read
Expand All @@ -18,4 +18,4 @@ jobs:
- uses: actions/setup-node@v4
with:
node-version: 20
- run: node --test AssignReviewers/
- run: node --test AssignReviewers/ ReviewConfig/
2 changes: 2 additions & 0 deletions .github/workflows/assign-reviewers.yml
Original file line number Diff line number Diff line change
Expand Up @@ -20,3 +20,5 @@ jobs:
- uses: StaflSystems/CustomGithubActions/AssignReviewers@main
with:
token: ${{ steps.app.outputs.token }}
pto-calendar-url: ${{ secrets.PTO_CALENDAR_URL }}
people: ${{ vars.REVIEW_PEOPLE }}
18 changes: 13 additions & 5 deletions AssignReviewers/README.md
Original file line number Diff line number Diff line change
Expand Up @@ -15,8 +15,8 @@ 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 whose GitHub status is Busy is skipped, and ties
rotate by PR number.
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.
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.
Expand All @@ -30,7 +30,11 @@ merge-queue PRs are skipped. Team review auto-assignment settings are left as th
so the action uses an installation token from the existing `staflsystemsci` app (org variable
`STAFL_CI_APP_ID`, org secret `STAFL_CI_PRIVATE_KEY`). It is installed on all repositories and
has the permissions needed: *Pull requests* and *Issues* write, *Members* read.
2. **Each repository**: add `.github/workflows/assign-reviewers.yml`:
2. **PTO calendar**: the Rippling PTO calendar feed URL is the org secret `PTO_CALENDAR_URL`. People
are matched to it by name through the org variable `REVIEW_PEOPLE` (see
[ReviewConfig](../ReviewConfig/README.md)), kept out of this public repo. Without both, nobody is
skipped.
3. **Each repository**: add `.github/workflows/assign-reviewers.yml`:

```yaml
name: Assign reviewers
Expand All @@ -51,9 +55,11 @@ merge-queue PRs are skipped. Team review auto-assignment settings are left as th
- uses: StaflSystems/CustomGithubActions/AssignReviewers@main
with:
token: ${{ steps.app.outputs.token }}
pto-calendar-url: ${{ secrets.PTO_CALENDAR_URL }}
people: ${{ vars.REVIEW_PEOPLE }}
```

3. **Authors** no longer need to add reviewers by hand, and shouldn't assign themselves.
4. **Authors** no longer need to add reviewers by hand, and shouldn't assign themselves.

## Inputs

Expand All @@ -64,9 +70,11 @@ merge-queue PRs are skipped. Team review auto-assignment settings are left as th
| `domain-team` | `embeddedreviewersstaff` | Team the domain approver is picked from |
| `review-team` | the rotation team | Team requested as reviewers |
| `max-stack-depth` | `30` | How far down a stack to look for assignees to inherit |
| `pto-calendar-url` | none | Rippling PTO calendar feed; people out today or next business day aren't picked |
| `people` | none | `REVIEW_PEOPLE` org variable: GitHub login to Rippling name and Slack ID |

## Tests

```bash
node --test AssignReviewers/
node --test AssignReviewers/ ReviewConfig/
```
14 changes: 14 additions & 0 deletions AssignReviewers/action.yml
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,16 @@ inputs:
max-stack-depth:
description: How many PRs down the stack to look for assignees to inherit.
default: '30'
pto-calendar-url:
description: >
Rippling PTO calendar feed (iCalendar) URL, from the PTO_CALENDAR_URL org secret. Anyone it
has out today or next business day isn't picked. Without it, nobody is skipped.
default: ''
people:
description: >
People map from the REVIEW_PEOPLE org variable, as JSON: each GitHub login's name as Rippling
shows it and their Slack member ID. Kept out of this public repo. Needed with pto-calendar-url.
default: ''

runs:
using: composite
Expand All @@ -32,6 +42,8 @@ runs:
DOMAIN_TEAM: ${{ inputs.domain-team }}
REVIEW_TEAM: ${{ inputs.review-team }}
MAX_STACK_DEPTH: ${{ inputs.max-stack-depth }}
PTO_CALENDAR_URL: ${{ inputs.pto-calendar-url }}
REVIEW_PEOPLE: ${{ inputs.people }}
with:
github-token: ${{ inputs.token }}
script: |
Expand All @@ -45,5 +57,7 @@ runs:
domainTeam: process.env.DOMAIN_TEAM,
reviewTeam: process.env.REVIEW_TEAM,
maxStackDepth: process.env.MAX_STACK_DEPTH,
ptoCalendarUrl: process.env.PTO_CALENDAR_URL,
people: process.env.REVIEW_PEOPLE,
},
});
39 changes: 17 additions & 22 deletions AssignReviewers/assign.js
Original file line number Diff line number Diff line change
Expand Up @@ -5,12 +5,14 @@
// - 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 whose GitHub status is Busy.
// already assigned to them, skipping anyone the PTO calendar has out today or next business day.
// - 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
// topped up, so reruns are harmless and hand-picked assignees are kept.

const { awaySoon } = require('../ReviewConfig/pto.js');

function isBot(user) {
return !user || user.type === 'Bot' || user.login.endsWith('[bot]');
}
Expand Down Expand Up @@ -52,25 +54,21 @@ 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, and whether
// their GitHub status says they're busy. One GraphQL query for all candidates.
async function loadAndAvailability(github, org, logins, core) {
// 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 {};
try {
return await queryLoadAndAvailability(github, org, logins);
return await queryLoad(github, org, logins);
} catch (error) {
core.warning(`Couldn't read reviewer load or status (${error.message}); picking without them.`);
return Object.fromEntries(logins.map((login) => [login, { load: 0, busy: false }]));
core.warning(`Couldn't read reviewer load (${error.message}); picking without it.`);
return Object.fromEntries(logins.map((login) => [login, 0]));
}
}

// 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.
async function queryLoadAndAvailability(github, org, logins) {
const users = logins
.map((login, i) => `u${i}: user(login: ${JSON.stringify(login)}) { status { indicatesLimitedAvailability } }`)
.join('\n');
async function queryLoad(github, org, logins) {
const prFields = 'pageInfo { hasNextPage endCursor } nodes { isDraft assignees(first: 10) { nodes { login } } }';
const data = await github.graphql(
`query($org: String!) {
Expand All @@ -79,7 +77,6 @@ async function queryLoadAndAvailability(github, org, logins) {
nodes { name pullRequests(states: OPEN, first: 100) { ${prFields} } }
}
}
${users}
}`,
{ org },
);
Expand Down Expand Up @@ -107,12 +104,7 @@ async function queryLoadAndAvailability(github, org, logins) {
count(page);
}
}
return Object.fromEntries(
logins.map((login, i) => [
login,
{ load: load[login], busy: Boolean(data[`u${i}`]?.status?.indicatesLimitedAvailability) },
]),
);
return load;
}

module.exports = async ({ github, context, core, inputs, sleep = (ms) => new Promise((r) => setTimeout(r, ms)) }) => {
Expand All @@ -125,6 +117,7 @@ module.exports = async ({ github, context, core, inputs, sleep = (ms) => new Pro
const maxDepth = Number(inputs.maxStackDepth || 30);
const parentWaitMs = Number(inputs.parentWaitMs ?? 120000);
const pollMs = Number(inputs.pollMs ?? 10000);
const pto = inputs.pto || {};

if (pr.draft) return core.info('Draft PR: nothing to do.');
if (isBot(pr.user)) return core.info(`Bot-authored PR (${author}): nothing to do.`);
Expand All @@ -151,15 +144,17 @@ module.exports = async ({ github, context, core, inputs, sleep = (ms) => new Pro
}

// 2. Fill the domain approver, then the rotation reviewer, least-loaded first.
let away;
const pick = async (team, members) => {
const candidates = members.filter((login) => login !== author && !named.has(login)).sort();
if (candidates.length === 0) return null;
const stats = await loadAndAvailability(github, owner, candidates, core);
const available = candidates.filter((login) => !stats[login].busy);
away ??= await awaySoon({ url: inputs.ptoCalendarUrl, people: inputs.people, core, ...pto });
const load = await reviewerLoad(github, owner, candidates, core);
const available = candidates.filter((login) => !away.has(login));
const pool = available.length > 0 ? available : candidates;
// Least loaded wins; ties rotate by PR number so they don't always go to the same person.
const least = Math.min(...pool.map((login) => stats[login].load));
const tied = pool.filter((login) => stats[login].load === least);
const least = Math.min(...pool.map((login) => load[login]));
const tied = pool.filter((login) => load[login] === least);
const chosen = tied[pr.number % tied.length];
notes.push(`picked ${chosen} from ${team} (${least} open assigned PRs)`);
return chosen;
Expand Down
91 changes: 78 additions & 13 deletions AssignReviewers/assign.test.js
Original file line number Diff line number Diff line change
Expand Up @@ -6,16 +6,16 @@ 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, per-user load and Busy status.
function fakeGithub({ prs, load = {}, busy = [] }) {
// A fake GitHub: open PRs keyed by number, team membership and per-user load.
function fakeGithub({ prs, load = {} }) {
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, plus Busy status per user.
graphql: async (query) => {
// Serves the load query from the fake PRs, as GitHub would.
graphql: async () => {
calls.graphql++;
const out = {
organization: {
Expand All @@ -39,9 +39,6 @@ function fakeGithub({ prs, load = {}, busy = [] }) {
},
},
};
for (const [, key, login] of query.matchAll(/(u\d+): user\(login: "([^"]+)"\)/g)) {
out[key] = { status: { indicatesLimitedAvailability: busy.includes(login) } };
}
return out;
},
rest: {
Expand Down Expand Up @@ -91,7 +88,37 @@ function fakeGithub({ prs, load = {}, busy = [] }) {
return { github, calls, pr: (n) => byNumber.get(n) };
}

function run(fake, number, overrides = {}, onSleep = () => {}) {
// A PTO calendar feed with all-day entries: [name, kind, first date, last date].
function ptoFeed(entries) {
const day = (d) => d.replaceAll('-', '');
const next = (d) => new Date(Date.parse(`${d}T12:00:00Z`) + 864e5).toISOString().slice(0, 10);
const events = entries.map(
([name, kind, first, last = first]) =>
`BEGIN:VEVENT\r\nDTSTART;VALUE=DATE:${day(first)}\r\nDTEND;VALUE=DATE:${day(next(last))}\r\nSUMMARY:${name} ${kind}\r\nEND:VEVENT`,
);
return ['BEGIN:VCALENDAR', ...events, 'END:VCALENDAR'].join('\r\n');
}

// Thursday Oct 1, 2026, 10:00 Pacific.
const THURSDAY = new Date('2026-10-01T17:00:00Z');
const PEOPLE = Object.fromEntries(EVERYONE.map((login) => [login, { name: `Person ${login}` }]));

function withPto(entries, fetches = []) {
return {
ptoCalendarUrl: 'https://calendar.example/pto.ics',
pto: {
now: THURSDAY,
people: PEOPLE,
holidays: { 2026: [] },
fetch: async (url) => {
fetches.push(url);
return { ok: true, status: 200, text: async () => ptoFeed(entries) };
},
},
};
}

function run(fake, number, overrides = {}, onSleep = () => {}, extraInputs = {}) {
const pr = fake.pr(number);
const logs = [];
const core = { info: (m) => logs.push(m), warning: (m) => logs.push(`WARN ${m}`) };
Expand All @@ -108,7 +135,7 @@ function run(fake, number, overrides = {}, onSleep = () => {}) {
},
},
};
const inputs = { rotationTeam: 'embeddedreviewers', domainTeam: 'embeddedreviewersstaff' };
const inputs = { rotationTeam: 'embeddedreviewers', domainTeam: 'embeddedreviewersstaff', ...extraInputs };
return assign({ github: fake.github, context, core, inputs, sleep: async () => onSleep() }).then(() => logs);
}

Expand Down Expand Up @@ -185,18 +212,56 @@ test('fills the domain approver when the stack only had a rotation assignee', as
assert.deepStrictEqual(fake.pr(41).assignees.sort(), ['devD', 'staffB']);
});

test('skips people whose GitHub status is Busy', async () => {
test('skips people the PTO calendar has out today or next business day', async () => {
const fake = fakeGithub({
prs: [{ number: 50, author: 'devE', head: 'a', base: 'main' }],
load: { staffA: 0, staffB: 3, staffC: 3, devD: 0, devF: 1 },
busy: ['staffA', 'devD'],
});
await run(fake, 50);
const pto = withPto([
['Person staffA', 'is Out of Office', '2026-10-01'],
['Person devD', 'on Vacation', '2026-10-02', '2026-10-09'],
]);
await run(fake, 50, {}, undefined, pto);
const assignees = fake.pr(50).assignees;
assert.ok(!assignees.includes('staffA') && !assignees.includes('devD'));
assert.ok(assignees.includes('devF'));
});

test('Work From Home and time off after the next business day do not count as out', async () => {
const fake = fakeGithub({
prs: [{ number: 51, author: 'devE', head: 'a', base: 'main' }],
load: { staffA: 0, staffB: 3, staffC: 3, devD: 0, devF: 1 },
});
const pto = withPto([
['Person staffA', 'on Work From Home', '2026-10-01'],
['Person devD', 'is Out of Office', '2026-10-05'],
]);
await run(fake, 51, {}, undefined, pto);
assert.deepStrictEqual(fake.pr(51).assignees, ['staffA', 'devD']);
});

test('still assigns, with a warning, when the PTO calendar cannot be read', async () => {
const fake = fakeGithub({ prs: [{ number: 52, author: 'devE', head: 'a', base: 'main' }] });
const pto = withPto([]);
pto.pto.fetch = async () => ({ ok: false, status: 403, text: async () => '' });
const logs = await run(fake, 52, {}, undefined, pto);
assert.strictEqual(fake.pr(52).assignees.length, 2);
assert.ok(logs.some((l) => l === "WARN Couldn't read the PTO calendar (HTTP 403); not checking who is out."));
assert.ok(logs.every((l) => !l.includes('calendar.example')));
});

test('does not read the PTO calendar when the stack already has its assignees', async () => {
const fake = fakeGithub({
prs: [
{ number: 53, author: 'devE', head: 'a', base: 'main', assignees: ['staffC', 'devD'] },
{ number: 54, author: 'devE', head: 'b', base: 'a' },
],
});
const fetches = [];
await run(fake, 54, {}, undefined, withPto([], fetches));
assert.deepStrictEqual(fetches, []);
});

test('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]) {
Expand Down Expand Up @@ -255,7 +320,7 @@ test('ignores drafts, bots and merge-queue PRs', async () => {
assert.deepStrictEqual(fake.calls, { assign: [], unassign: [], review: [], graphql: 0 });
});

test('still assigns when the load and status query fails', async () => {
test('still assigns when the load query fails', async () => {
const fake = fakeGithub({ prs: [{ number: 95, author: 'devE', head: 'a', base: 'main' }] });
fake.github.graphql = async () => {
throw new Error('Resource not accessible by integration');
Expand Down
Loading
Loading