From 256237d606ed0d233dab1eb9a80a25a25e06ef3a Mon Sep 17 00:00:00 2001 From: Thomas Willson Date: Thu, 1 Oct 2026 16:59:26 -0700 Subject: [PATCH 1/5] Add the scheduled ReviewSLA job: comments, reminders, digest, dashboard Step 2 of Proposal 2's rollout. Every 30 minutes during business hours the job rebuilds each assignee's clock from the open PRs' timelines (first response with the stack rule, and re-review after a push and re-request), then, by REVIEW_SLA_MODE: - shadow: a Review SLA comment on each PR, the Confluence dashboard (saved only when it changes, as a minor edit) and the 10:00 Slack digest - remind: also a Slack DM when a review is due, recorded in the comment's hidden marker so it's sent once, and reassignment across the stack of an assignee the PTO calendar has out before their review is due The least-loaded pick moves from AssignReviewers to ReviewConfig/pick.js so reassignment picks the way assignment does, and pto.js gains readOutDates for each person's out dates. Co-Authored-By: Claude Opus 5.5 --- .github/workflows/review-sla.yml | 59 ++++++++ AssignReviewers/assign.js | 73 +-------- ReviewConfig/README.md | 1 + ReviewConfig/hours.js | 2 +- ReviewConfig/pick.js | 76 ++++++++++ ReviewConfig/pto.js | 49 +++--- ReviewSLA/README.md | 80 +++++++++- ReviewSLA/action.yml | 111 ++++++++++++++ ReviewSLA/clock.js | 4 +- ReviewSLA/comment.js | 63 ++++++++ ReviewSLA/confluence.js | 65 ++++++++ ReviewSLA/github.js | 132 ++++++++++++++++ ReviewSLA/owed.js | 118 +++++++++++++++ ReviewSLA/owed.test.js | 149 ++++++++++++++++++ ReviewSLA/run.js | 235 +++++++++++++++++++++++++++++ ReviewSLA/run.test.js | 251 +++++++++++++++++++++++++++++++ ReviewSLA/slack.js | 41 +++++ 17 files changed, 1411 insertions(+), 98 deletions(-) create mode 100644 .github/workflows/review-sla.yml create mode 100644 ReviewConfig/pick.js create mode 100644 ReviewSLA/action.yml create mode 100644 ReviewSLA/comment.js create mode 100644 ReviewSLA/confluence.js create mode 100644 ReviewSLA/github.js create mode 100644 ReviewSLA/owed.js create mode 100644 ReviewSLA/owed.test.js create mode 100644 ReviewSLA/run.js create mode 100644 ReviewSLA/run.test.js create mode 100644 ReviewSLA/slack.js diff --git a/.github/workflows/review-sla.yml b/.github/workflows/review-sla.yml new file mode 100644 index 0000000..2dfba28 --- /dev/null +++ b/.github/workflows/review-sla.yml @@ -0,0 +1,59 @@ +name: Review SLA + +on: + schedule: + # Every 30 minutes, 10:00 to 17:30 Pacific on weekdays, in daylight and standard time. The job + # skips runs outside business hours and on holidays. + - cron: '*/30 17-23 * * 1-5' + - cron: '*/30 0-1 * * 2-6' + # 10:00 Pacific, which posts the Slack digest: 17:00 UTC in daylight time, 18:00 in standard. + - cron: '0 17 * * 1-5' + - cron: '0 18 * * 1-5' + workflow_dispatch: + inputs: + digest: + description: Post the Slack digest on this run + type: boolean + default: false + +permissions: {} + +# Runs never overlap, so a reminder recorded by one is seen by the next. +concurrency: + group: review-sla + cancel-in-progress: false + +jobs: + sla: + # REVIEW_SLA_MODE (off, shadow, remind or enforce) switches it, and is the rollback. + if: ${{ vars.REVIEW_SLA_MODE != '' && vars.REVIEW_SLA_MODE != 'off' }} + runs-on: ubuntu-latest + steps: + - uses: actions/checkout@v4.1.2 + - uses: actions/create-github-app-token@v2 + id: app + with: + app-id: ${{ vars.STAFL_CI_APP_ID }} + private-key: ${{ secrets.STAFL_CI_PRIVATE_KEY }} + owner: StaflSystems + - uses: ./ReviewSLA + with: + token: ${{ steps.app.outputs.token }} + mode: ${{ vars.REVIEW_SLA_MODE }} + repos: >- + StaflLib + coit-tower-bms2000 + goldengate-bms2000-string + stafl-bms2000-template + anza-bms2000 + CustomGithubActions + pto-calendar-url: ${{ secrets.PTO_CALENDAR_URL }} + people: ${{ vars.REVIEW_PEOPLE }} + slack-token: ${{ secrets.REVIEW_SLA_SLACK_BOT_TOKEN }} + slack-channel: ${{ vars.REVIEW_SLA_SLACK_CHANNEL }} + confluence-url: ${{ vars.CONFLUENCE_URL }} + confluence-user: ${{ vars.CONFLUENCE_USER }} + confluence-token: ${{ secrets.CONFLUENCE_API_TOKEN }} + confluence-page-id: ${{ vars.REVIEW_SLA_PAGE_ID }} + dashboard-url: https://staflsystems.atlassian.net/wiki/spaces/EM/pages/${{ vars.REVIEW_SLA_PAGE_ID }} + digest: ${{ inputs.digest || 'false' }} diff --git a/AssignReviewers/assign.js b/AssignReviewers/assign.js index c93b2f3..648906c 100644 --- a/AssignReviewers/assign.js +++ b/AssignReviewers/assign.js @@ -11,6 +11,7 @@ // - 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 { pickLeastLoaded, teamMembers } = require('../ReviewConfig/pick.js'); const { awaySoon } = require('../ReviewConfig/pto.js'); function isBot(user) { @@ -49,64 +50,6 @@ async function inheritedAssignees(github, owner, repo, pr, author, { maxDepth, p return null; } -async function teamMembers(github, org, team) { - const members = await github.paginate(github.rest.teams.listMembersInOrg, { org, team_slug: team, per_page: 100 }); - 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 {}; - try { - return 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 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 queryLoad(github, org, logins) { - const prFields = 'pageInfo { hasNextPage endCursor } nodes { isDraft assignees(first: 10) { nodes { login } } }'; - const data = await github.graphql( - `query($org: String!) { - organization(login: $org) { - repositories(first: 50, orderBy: { field: PUSHED_AT, direction: DESC }) { - nodes { name pullRequests(states: OPEN, first: 100) { ${prFields} } } - } - } - }`, - { org }, - ); - const load = Object.fromEntries(logins.map((login) => [login, 0])); - const count = (pullRequests) => { - 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]++; - } - } - }; - for (const repository of data.organization?.repositories?.nodes ?? []) { - let page = repository.pullRequests; - count(page); - // Busy repos (StaflLib) have more than one page of open PRs. - while (page?.pageInfo?.hasNextPage) { - const next = await github.graphql( - `query($org: String!, $name: String!, $after: String!) { - repository(owner: $org, name: $name) { pullRequests(states: OPEN, first: 100, after: $after) { ${prFields} } } - }`, - { org, name: repository.name, after: page.pageInfo.endCursor }, - ); - page = next.repository?.pullRequests; - count(page); - } - } - return load; -} - module.exports = async ({ github, context, core, inputs, sleep = (ms) => new Promise((r) => setTimeout(r, ms)) }) => { const { owner, repo } = context.repo; const pr = context.payload.pull_request; @@ -146,18 +89,12 @@ 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(); + const candidates = members.filter((login) => login !== author && !named.has(login)); if (candidates.length === 0) return null; 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) => 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; + 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)`); + return chosen.login; }; if (!hasDomainApprover()) { const chosen = await pick(domainTeam, domainMembers); diff --git a/ReviewConfig/README.md b/ReviewConfig/README.md index dd80e0a..1982e14 100644 --- a/ReviewConfig/README.md +++ b/ReviewConfig/README.md @@ -27,6 +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 | — | ## Who is out diff --git a/ReviewConfig/hours.js b/ReviewConfig/hours.js index c14284c..f4f1be5 100644 --- a/ReviewConfig/hours.js +++ b/ReviewConfig/hours.js @@ -60,4 +60,4 @@ function addBusinessHours(start, hours, holidays) { } } -module.exports = { addBusinessHours, businessHoursBetween, isBusinessDay, pacificTime }; +module.exports = { addBusinessHours, businessHoursBetween, isBusinessDay, pacificOffset, pacificTime }; diff --git a/ReviewConfig/pick.js b/ReviewConfig/pick.js new file mode 100644 index 0000000..b883871 --- /dev/null +++ b/ReviewConfig/pick.js @@ -0,0 +1,76 @@ +// Picking a reviewer: the least-loaded available member of a team. Shared by AssignReviewers, +// which picks a new PR's assignees, and ReviewSLA, which replaces an assignee who is out. + +async function teamMembers(github, org, team) { + const members = await github.paginate(github.rest.teams.listMembersInOrg, { org, team_slug: team, per_page: 100 }); + 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 {}; + try { + return 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 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 queryLoad(github, org, logins) { + const prFields = 'pageInfo { hasNextPage endCursor } nodes { isDraft assignees(first: 10) { nodes { login } } }'; + const data = await github.graphql( + `query($org: String!) { + organization(login: $org) { + repositories(first: 50, orderBy: { field: PUSHED_AT, direction: DESC }) { + nodes { name pullRequests(states: OPEN, first: 100) { ${prFields} } } + } + } + }`, + { org }, + ); + const load = Object.fromEntries(logins.map((login) => [login, 0])); + const count = (pullRequests) => { + 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]++; + } + } + }; + for (const repository of data.organization?.repositories?.nodes ?? []) { + let page = repository.pullRequests; + count(page); + // Busy repos (StaflLib) have more than one page of open PRs. + while (page?.pageInfo?.hasNextPage) { + const next = await github.graphql( + `query($org: String!, $name: String!, $after: String!) { + repository(owner: $org, name: $name) { pullRequests(states: OPEN, first: 100, after: $after) { ${prFields} } } + }`, + { org, name: repository.name, after: page.pageInfo.endCursor }, + ); + page = next.repository?.pullRequests; + count(page); + } + } + return load; +} + +// 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. +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 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 }; +} + +module.exports = { pickLeastLoaded, reviewerLoad, teamMembers }; diff --git a/ReviewConfig/pto.js b/ReviewConfig/pto.js index c335521..a384f2c 100644 --- a/ReviewConfig/pto.js +++ b/ReviewConfig/pto.js @@ -125,46 +125,35 @@ function outDates(icsText, people) { return out; } -// The logins who are out today or on the next business day (Pacific): a review assigned now is -// due within about one business day, so someone out tomorrow shouldn't get it today. +// For each login in the people map, the Pacific dates the PTO calendar has them out, or null with a +// warning when the feed or the map can't be read. With no URL configured, it's null with a note. // // `people` is the people map, as an object or as the JSON text of the REVIEW_PEOPLE org variable: // { "": { "name": "", "aliases": [""], // "slack": "" } }, with aliases optional. -// -// Returns an empty set, with a warning, when the feed or the map can't be read: picking someone who -// is out is better than not assigning at all. With no URL configured, it does nothing. -async function awaySoon({ - url, - core, - people, - now = new Date(), - fetch = globalThis.fetch, - holidays = readHolidays(), -}) { +async function readOutDates({ url, core, people, now = new Date(), fetch = globalThis.fetch, holidays = readHolidays() }) { if (!url) { core.info('No PTO calendar configured; not checking who is out.'); - return new Set(); + return null; } try { people = typeof people === 'string' ? JSON.parse(people || '{}') : people || {}; } catch { core.warning("The REVIEW_PEOPLE org variable isn't valid JSON; not checking who is out."); - return new Set(); + return null; } if (Object.keys(people).length === 0) { core.warning('The REVIEW_PEOPLE org variable is empty or not passed in; not checking who is out.'); - return new Set(); + return null; } const unnamed = Object.entries(people).filter(([, person]) => namesOf(person).length === 0).map(([login]) => login); if (unnamed.length > 0) { core.warning(`No Rippling name in REVIEW_PEOPLE for ${unnamed.join(', ')}, so their PTO isn't checked.`); } - const today = pacificDate(now); - if (!holidaysFor(holidays, today.slice(0, 4))) { - core.warning(`ReviewConfig/holidays.json has no holidays for ${today.slice(0, 4)}; every weekday counts as a business day.`); + const year = pacificDate(now).slice(0, 4); + if (!holidaysFor(holidays, year)) { + core.warning(`ReviewConfig/holidays.json has no holidays for ${year}; every weekday counts as a business day.`); } - const window = [today, nextBusinessDay(today, holidays)]; let text; try { // The URL carries a token, so neither it nor an error that might quote it is logged. @@ -173,11 +162,24 @@ async function awaySoon({ text = await response.text(); } catch (error) { core.warning(`Couldn't read the PTO calendar (${/^HTTP \d+$/.test(error.message) ? error.message : error.name}); not checking who is out.`); - return new Set(); + return null; } - const dates = outDates(text, people); + return outDates(text, people); +} + +// The logins who are out today or on the next business day (Pacific): a review assigned now is +// due within about one business day, so someone out tomorrow shouldn't get it today. +// +// Returns an empty set when the feed or the map can't be read: picking someone who is out is +// better than not assigning at all. +async function awaySoon(options) { + const { now = new Date(), holidays = readHolidays() } = options; + const dates = await readOutDates({ ...options, now, holidays }); + if (!dates) return new Set(); + const today = pacificDate(now); + const window = [today, nextBusinessDay(today, holidays)]; const away = new Set(Object.keys(dates).filter((login) => window.some((d) => dates[login].has(d)))); - core.info(`Out ${window.join(' or ')}: ${[...away].join(', ') || 'nobody'}.`); + options.core.info(`Out ${window.join(' or ')}: ${[...away].join(', ') || 'nobody'}.`); return away; } @@ -192,4 +194,5 @@ module.exports = { pacificDate, parseIcs, readHolidays, + readOutDates, }; diff --git a/ReviewSLA/README.md b/ReviewSLA/README.md index dac75fc..3c0a071 100644 --- a/ReviewSLA/README.md +++ b/ReviewSLA/README.md @@ -1,8 +1,76 @@ # Review SLA -First-response clocks for the review throughput RFC's Proposal 2. This is step 1 of its rollout: -the clock code and report mode. Nothing is scheduled yet, so nothing changes for authors or -reviewers. Reminders, the Confluence dashboard and reassignment come in later steps. +First-response clocks for the review throughput RFC's Proposal 2: a scheduled job that works out +who owes a review on every open PR and by when, and report mode, which measures it over past weeks. + +## The scheduled job + +`.github/workflows/review-sla.yml` runs it every 30 minutes during business hours (10:00 to 17:00 +Pacific, business days), and on demand. Each run rebuilds every assignee's clock from the open +PRs' timelines; there's no other state. The repo variable `REVIEW_SLA_MODE` switches it, and is +the rollback for every step: + +| Mode | What it does | +| --- | --- | +| `off`, or unset | Nothing; the job is skipped | +| `shadow` | Keeps each PR's Review SLA comment, the Confluence dashboard and the 10:00 Slack digest up to date | +| `remind` | Also DMs each assignee when their review is due, and reassigns anyone the PTO calendar has out before their review is due | +| `enforce` | Reassignment at twice the target. Not built yet; runs as `remind` | + +### Who owes what + +Each assignee other than the author has their own clock on each ready PR: + +- **First response:** due 4 business hours after it starts on a PR under 250 added lines, 7 (one + business day) otherwise. It starts at the latest of the PR being marked ready, the person being + assigned, and, mid-stack, their approval of the PR below or the PR below merging. It stops at + their first review that counts: an approval, changes requested, or a comment review with a body + or at least one inline comment. +- **Stacks:** an assignee owes a response only on the lowest ready PR in the stack they're + assigned to and haven't approved. Requesting changes doesn't move them up the stack. +- **Re-review:** due 4 business hours after the author re-requests their review, if the author + has pushed since their last one. A re-request with no new push, as Graphite makes on every + `gt submit`, starts nothing. +- **Drafts** stop every clock on the PR. Marking it ready again starts the first-response clocks + over. + +Bot PRs and Graphite merge-queue PRs have no clocks. + +### What it changes + +- **A Review SLA comment** on each ready PR where someone owes a review, edited in place: who owes + what, and when it's due. A hidden marker in it records which reminders have been sent, so reruns + and late scheduled runs don't send them twice. The comment is written before the DM, so a failed + write means a missed reminder, not a repeated one. +- **Reminder DMs** (`remind`): one per clock, when it's due, to the assignee's Slack ID in + `REVIEW_PEOPLE`. +- **Out-of-office reassignment** (`remind`): an assignee the PTO calendar has out on any day from + today to the day their review is due is replaced on every PR in the stack they're assigned to. + A domain approver is replaced from the domain team and a rotation reviewer from the rotation team, + by the same least-loaded pick AssignReviewers uses (`ReviewConfig/pick.js`). The other assignee is + kept, and a comment on the lowest of those PRs says who took over and why. +- **The digest:** posted to the `REVIEW_SLA_SLACK_CHANNEL` channel by the 10:00 Pacific run + (or a manual run with **digest** ticked): overdue reviews by person, and how many more are due + today. +- **The dashboard:** a Confluence page with a checkmark per person while they're meeting the SLA, + and their overdue reviews when they aren't. It saves a new version only when the content + changes, as a minor edit, so watchers aren't notified every 30 minutes. + +It fails safe: an API error is a warning, and that PR, repo, channel or page is skipped this run. + +### Setup + +| Name | Kind | What it is | +| --- | --- | --- | +| `REVIEW_SLA_MODE` | Repo variable | `off`, `shadow`, `remind` or `enforce` | +| `STAFL_CI_APP_ID`, `STAFL_CI_PRIVATE_KEY` | Org variable, secret | The staflsystemsci app, installed on every repo checked, with Pull requests and Issues write and Members read | +| `PTO_CALENDAR_URL` | Org secret | Rippling PTO calendar feed | +| `REVIEW_PEOPLE` | Org variable | Names and Slack IDs (see `ReviewConfig/`) | +| `REVIEW_SLA_SLACK_BOT_TOKEN`, `REVIEW_SLA_SLACK_CHANNEL` | Org secret, variable | Slack app with `chat:write`, invited to the digest channel | +| `CONFLUENCE_URL`, `CONFLUENCE_USER`, `CONFLUENCE_API_TOKEN` | Org variables, secret | Base URL ending in `/wiki`, and the email and API token of an account that can edit the page. A scoped (service account) token uses `https://api.atlassian.com/ex/confluence//wiki` | +| `REVIEW_SLA_PAGE_ID` | Org variable | The dashboard page | + +The repos checked are listed in the workflow. ## Report mode @@ -55,7 +123,11 @@ PRs by creation date, and 34 PRs opened before Jun 26 were marked ready during t | File | What it does | | --- | --- | -| `clock.js` | Rebuilds a PR's first-response clock from its ready and draft events and its reviews | +| `run.js` | The scheduled job: modes, out-of-office reassignment, comments, reminders, digest, dashboard | +| `owed.js` | Who owes a review now: each assignee's clock, with the stack and re-review rules | +| `github.js` | Reads each repo's open PRs and their timelines | +| `comment.js`, `slack.js`, `confluence.js` | The PR comment, Slack messages and the dashboard page | +| `clock.js` | A PR's first response, measured both ways, for report mode | | `report.js` | Report mode: fetches PRs and writes the weekly report | ## Tests diff --git a/ReviewSLA/action.yml b/ReviewSLA/action.yml new file mode 100644 index 0000000..1b9df04 --- /dev/null +++ b/ReviewSLA/action.yml @@ -0,0 +1,111 @@ +name: Review SLA +description: > + Works out who owes a review on every open PR and by when, keeps a Review SLA comment on each PR, + a Confluence dashboard and a 10:00 Slack digest up to date, DMs assignees when a review is due, + and reassigns an assignee who is out before it's due. + +inputs: + token: + description: > + GitHub App installation token covering every repo in `repos`, with repository permissions Pull + requests (write) and Issues (write), and organization permission Members (read). + required: true + mode: + description: "off, shadow, remind or enforce. Usually the REVIEW_SLA_MODE variable." + default: 'off' + repos: + description: Repositories to check, separated by spaces or newlines. + required: true + owner: + description: Organization the repositories are in. + default: StaflSystems + rotation-team: + description: Team a rotation reviewer who is out is replaced from. + default: embeddedreviewers + domain-team: + description: Team a domain approver who is out is replaced from. + default: embeddedreviewersstaff + pto-calendar-url: + description: Rippling PTO calendar feed URL, from the PTO_CALENDAR_URL org secret. Never logged. + default: '' + people: + description: People map from the REVIEW_PEOPLE org variable (names and Slack IDs). + default: '' + slack-token: + description: Slack bot token, with chat:write. Without it, no reminders or digest. + default: '' + slack-channel: + description: Slack channel ID for the digest. The bot has to be invited to it. + default: '' + confluence-url: + description: > + Confluence base URL, ending in /wiki: https://.atlassian.net/wiki, or for a scoped + (service account) API token, https://api.atlassian.com/ex/confluence//wiki. + default: '' + confluence-user: + description: Email address of the Atlassian account the API token belongs to. + default: '' + confluence-token: + description: Atlassian API token that can edit the dashboard page. + default: '' + confluence-page-id: + description: ID of the dashboard page. + default: '' + dashboard-url: + description: Link to the dashboard, for PR comments and the digest. + default: '' + digest: + description: "'true' posts the Slack digest on this run, whatever the time." + default: 'false' + +runs: + using: composite + steps: + - uses: actions/github-script@v8 + env: + ACTION_PATH: ${{ github.action_path }} + EVENT_NAME: ${{ github.event_name }} + EVENT_SCHEDULE: ${{ github.event.schedule }} + MODE: ${{ inputs.mode }} + REPOS: ${{ inputs.repos }} + OWNER: ${{ inputs.owner }} + ROTATION_TEAM: ${{ inputs.rotation-team }} + DOMAIN_TEAM: ${{ inputs.domain-team }} + PTO_CALENDAR_URL: ${{ inputs.pto-calendar-url }} + REVIEW_PEOPLE: ${{ inputs.people }} + SLACK_TOKEN: ${{ inputs.slack-token }} + SLACK_CHANNEL: ${{ inputs.slack-channel }} + CONFLUENCE_URL: ${{ inputs.confluence-url }} + CONFLUENCE_USER: ${{ inputs.confluence-user }} + CONFLUENCE_TOKEN: ${{ inputs.confluence-token }} + CONFLUENCE_PAGE_ID: ${{ inputs.confluence-page-id }} + DASHBOARD_URL: ${{ inputs.dashboard-url }} + DIGEST: ${{ inputs.digest }} + with: + github-token: ${{ inputs.token }} + script: | + const run = require(`${process.env.ACTION_PATH}/run.js`); + const env = process.env; + await run({ + github, + core, + inputs: { + eventName: env.EVENT_NAME, + schedule: env.EVENT_SCHEDULE, + mode: env.MODE, + repos: env.REPOS, + owner: env.OWNER, + rotationTeam: env.ROTATION_TEAM, + domainTeam: env.DOMAIN_TEAM, + ptoCalendarUrl: env.PTO_CALENDAR_URL, + people: env.REVIEW_PEOPLE, + slackToken: env.SLACK_TOKEN, + slackChannel: env.SLACK_CHANNEL, + confluenceUrl: env.CONFLUENCE_URL, + confluenceUser: env.CONFLUENCE_USER, + confluenceToken: env.CONFLUENCE_TOKEN, + confluencePageId: env.CONFLUENCE_PAGE_ID, + dashboardUrl: env.DASHBOARD_URL, + digest: env.DIGEST, + }, + }); diff --git a/ReviewSLA/clock.js b/ReviewSLA/clock.js index 788b103..93d810d 100644 --- a/ReviewSLA/clock.js +++ b/ReviewSLA/clock.js @@ -17,7 +17,7 @@ const { businessHoursBetween } = require('../ReviewConfig/hours.js'); const SMALL_PR_LINES = 250; -const TARGET_HOURS = { small: 4, standard: 7 }; +const TARGET_HOURS = { small: 4, standard: 7, reReview: 4 }; const HOUR_MS = 3600 * 1000; function isBot(actor) { @@ -91,4 +91,4 @@ function firstResponse(pr, { until, holidays }) { return { rfc: rfcClock(pr, until), sla: slaClock(pr, until, holidays) }; } -module.exports = { countsAsResponse, firstResponse, hasSla, isBot, targetHours, SMALL_PR_LINES, TARGET_HOURS }; +module.exports = { byTime, countsAsResponse, firstResponse, hasSla, isBot, openedAsDraft, targetHours, SMALL_PR_LINES, TARGET_HOURS }; diff --git a/ReviewSLA/comment.js b/ReviewSLA/comment.js new file mode 100644 index 0000000..643d5cc --- /dev/null +++ b/ReviewSLA/comment.js @@ -0,0 +1,63 @@ +// The one Review SLA comment on each ready PR: who owes what, and when it's due. Its first line +// is a hidden marker that also records which reminders have been sent, so reruns and late +// scheduled runs don't send them twice. + +const { TIME_ZONE } = require('../ReviewConfig/pto.js'); +const { SMALL_PR_LINES, TARGET_HOURS } = require('./clock.js'); + +const MARKER = '`).exec(body); + try { + const state = match ? JSON.parse(match[1]) : {}; + return { reminded: Array.isArray(state.reminded) ? state.reminded : [] }; + } catch { + return { reminded: [] }; + } +} + +function renderComment(clocks, state, { dashboardUrl } = {}) { + const lines = [`${MARKER} ${JSON.stringify(state)} -->`, '**Review SLA**', '']; + if (clocks.length === 0) { + lines.push("Nobody owes a review on this PR right now."); + } else { + lines.push('| Assignee | Owes | Due | |', '| --- | --- | --- | --- |'); + for (const clock of [...clocks].sort((a, b) => a.due - b.due || a.login.localeCompare(b.login))) { + lines.push(`| \`${clock.login}\` | ${KIND[clock.kind]} | ${formatTime(clock.due)} PT | ${clock.overdue ? 'Overdue' : ''} |`); + } + } + lines.push( + '', + `Due times are in business hours, 10:00 to 17:00 Pacific: ${TARGET_HOURS.small} h for a PR under ${SMALL_PR_LINES} added lines, ` + + `${TARGET_HOURS.standard} h otherwise, and ${TARGET_HOURS.reReview} h for a re-review. Mid-stack, an assignee owes a response on the ` + + `lowest PR they haven't approved.${dashboardUrl ? ` [Dashboard](${dashboardUrl})` : ''}`, + ); + return lines.join('\n'); +} + +module.exports = { formatTime, KIND, MARKER, parseState, reminderKey, renderComment }; diff --git a/ReviewSLA/confluence.js b/ReviewSLA/confluence.js new file mode 100644 index 0000000..63938f2 --- /dev/null +++ b/ReviewSLA/confluence.js @@ -0,0 +1,65 @@ +// The Review SLA dashboard: a Confluence page under Embedded Team Dashboard, rewritten each run. +// It shows a checkmark per person while they're meeting the SLA, and lists overdue reviews only for +// those who aren't. A new version is saved only when the content changes, as a minor edit, so +// neither the page history nor watchers' notifications fill up every 30 minutes. The version +// message carries a hash of the content, which is what "changed" is checked against: Confluence +// rewrites storage format on save, so the page's own body can't be compared. + +const crypto = require('node:crypto'); +const { formatTime, KIND } = require('./comment.js'); +const { SMALL_PR_LINES, TARGET_HOURS } = require('./clock.js'); + +const html = (text) => String(text).replace(/&/g, '&').replace(//g, '>').replace(/"/g, '"'); + +function renderDashboard(clocks, { people }) { + const nameOf = (login) => people[login]?.name || login; + const logins = [...new Set([...Object.keys(people), ...clocks.map((c) => c.login)])].sort((a, b) => nameOf(a).localeCompare(nameOf(b))); + const rows = logins.map((login) => { + const mine = clocks.filter((c) => c.login === login); + const overdue = mine.filter((c) => c.overdue).sort((a, b) => a.due - b.due); + const status = overdue.length === 0 ? '✅' : `${overdue.length} overdue`; + const list = overdue.length === 0 ? '' : ``; + return `${html(nameOf(login))}${status}${mine.length}${list}`; + }); + return [ + '

Kept up to date by the Review SLA job every 30 minutes during business hours (10:00 to 17:00 Pacific, business days). ' + + `An assignee owes a first response within ${TARGET_HOURS.small} business hours on a PR under ${SMALL_PR_LINES} added lines, ` + + `${TARGET_HOURS.standard} otherwise, and a re-review within ${TARGET_HOURS.reReview}. Times are Pacific.

`, + '', + ...rows, + '
AssigneeStatusReviews owedOverdue
', + ].join(''); +} + +// `baseUrl` ends in /wiki: https://.atlassian.net/wiki, or for a scoped (service account) +// token, https://api.atlassian.com/ex/confluence//wiki. +async function updateDashboard({ baseUrl, user, token, pageId, body, fetch = globalThis.fetch }) { + const headers = { + authorization: `Basic ${Buffer.from(`${user}:${token}`).toString('base64')}`, + accept: 'application/json', + 'content-type': 'application/json', + }; + const url = `${baseUrl.replace(/\/$/, '')}/rest/api/content/${pageId}`; + const current = await fetch(`${url}?expand=version`, { headers }); + if (!current.ok) throw new Error(`Confluence: reading the page returned HTTP ${current.status}`); + const page = await current.json(); + const message = `review-sla ${crypto.createHash('sha256').update(body).digest('hex').slice(0, 16)}`; + if (page.version?.message === message) return false; + const update = await fetch(url, { + method: 'PUT', + headers, + body: JSON.stringify({ + id: pageId, + type: 'page', + title: page.title, + version: { number: page.version.number + 1, minorEdit: true, message }, + body: { storage: { value: body, representation: 'storage' } }, + }), + }); + if (!update.ok) throw new Error(`Confluence: updating the page returned HTTP ${update.status}`); + return true; +} + +module.exports = { renderDashboard, updateDashboard }; diff --git a/ReviewSLA/github.js b/ReviewSLA/github.js new file mode 100644 index 0000000..4417809 --- /dev/null +++ b/ReviewSLA/github.js @@ -0,0 +1,132 @@ +// Reads a repo's open PRs, with the timeline events owed.js needs and the PR's Review SLA comment. + +const { MARKER } = require('./comment.js'); + +const TIMELINE_TYPES = [ + 'READY_FOR_REVIEW_EVENT', + 'CONVERT_TO_DRAFT_EVENT', + 'PULL_REQUEST_REVIEW', + 'ASSIGNED_EVENT', + 'UNASSIGNED_EVENT', + 'REVIEW_REQUESTED_EVENT', + 'HEAD_REF_FORCE_PUSHED_EVENT', + 'PULL_REQUEST_COMMIT', + 'BASE_REF_CHANGED_EVENT', +].join(', '); + +const TIMELINE_FIELDS = ` + pageInfo { hasPreviousPage startCursor } + nodes { + __typename + ... on ReadyForReviewEvent { createdAt } + ... on ConvertToDraftEvent { createdAt } + ... on PullRequestReview { state submittedAt body comments { totalCount } author { __typename login } } + ... on AssignedEvent { createdAt assignee { ... on User { login } } } + ... on UnassignedEvent { createdAt assignee { ... on User { login } } } + ... on ReviewRequestedEvent { createdAt requestedReviewer { ... on User { login } } } + ... on HeadRefForcePushedEvent { createdAt } + ... on PullRequestCommit { commit { committedDate } } + ... on BaseRefChangedEvent { createdAt } + }`; + +const OPEN_PRS = `query($owner: String!, $name: String!, $after: String) { + repository(owner: $owner, name: $name) { + pullRequests(states: OPEN, first: 30, after: $after) { + pageInfo { hasNextPage endCursor } + nodes { + number url title isDraft additions headRefName baseRefName createdAt + author { __typename login } + assignees(first: 10) { nodes { login } } + comments(last: 50) { nodes { databaseId body } } + timelineItems(last: 100, itemTypes: [${TIMELINE_TYPES}]) { ${TIMELINE_FIELDS} } + } + } + } +}`; + +const EARLIER_TIMELINE = `query($owner: String!, $name: String!, $number: Int!, $before: String!) { + repository(owner: $owner, name: $name) { + pullRequest(number: $number) { + timelineItems(last: 100, before: $before, itemTypes: [${TIMELINE_TYPES}]) { ${TIMELINE_FIELDS} } + } + } +}`; + +const actor = (a) => a && { login: a.login, bot: a.__typename === 'Bot' }; + +function toEvent(item) { + const at = (iso) => (iso ? new Date(iso) : null); + switch (item.__typename) { + case 'ReadyForReviewEvent': + return { type: 'ready', at: at(item.createdAt) }; + case 'ConvertToDraftEvent': + return { type: 'draft', at: at(item.createdAt) }; + case 'PullRequestReview': + // Pending reviews have no submittedAt and aren't visible to anyone else yet. + return item.submittedAt && { + type: 'review', + at: at(item.submittedAt), + author: actor(item.author), + state: item.state, + body: item.body, + comments: item.comments.totalCount, + }; + case 'AssignedEvent': + case 'UnassignedEvent': + return item.assignee?.login && { type: item.__typename === 'AssignedEvent' ? 'assigned' : 'unassigned', at: at(item.createdAt), login: item.assignee.login }; + case 'ReviewRequestedEvent': + return item.requestedReviewer?.login && { type: 'requested', at: at(item.createdAt), login: item.requestedReviewer.login }; + case 'HeadRefForcePushedEvent': + return { type: 'pushed', at: at(item.createdAt) }; + case 'PullRequestCommit': + return item.commit?.committedDate && { type: 'pushed', at: at(item.commit.committedDate) }; + case 'BaseRefChangedEvent': + return { type: 'base', at: at(item.createdAt) }; + default: + return null; + } +} + +function toOpenPr(repo, node, items) { + const sticky = node.comments.nodes.find((c) => c.body?.startsWith(MARKER)); + return { + repo, + number: node.number, + url: node.url, + title: node.title, + isDraft: node.isDraft, + additions: node.additions, + headRefName: node.headRefName, + baseRefName: node.baseRefName, + createdAt: new Date(node.createdAt), + closedAt: null, + author: actor(node.author) ?? { login: 'ghost', bot: false }, + assignees: node.assignees.nodes.map((a) => a.login), + timeline: items.map(toEvent).filter(Boolean), + sticky: sticky ? { id: sticky.databaseId, body: sticky.body } : null, + }; +} + +// Earlier timeline pages are read for PRs with more than 100 events, up to `maxPages` in all. +async function fetchOpenPrs({ github, owner, repo, maxPages = 5 }) { + const prs = []; + for (let after = null; ; ) { + const data = await github.graphql(OPEN_PRS, { owner, name: repo, after }); + const page = data.repository.pullRequests; + for (const node of page.nodes) { + let items = node.timelineItems.nodes; + let info = node.timelineItems.pageInfo; + for (let pages = 1; info.hasPreviousPage && pages < maxPages; pages++) { + const earlier = await github.graphql(EARLIER_TIMELINE, { owner, name: repo, number: node.number, before: info.startCursor }); + items = [...earlier.repository.pullRequest.timelineItems.nodes, ...items]; + info = earlier.repository.pullRequest.timelineItems.pageInfo; + } + prs.push(toOpenPr(repo, node, items)); + } + if (!page.pageInfo.hasNextPage) break; + after = page.pageInfo.endCursor; + } + return prs; +} + +module.exports = { fetchOpenPrs, toEvent, toOpenPr }; diff --git a/ReviewSLA/owed.js b/ReviewSLA/owed.js new file mode 100644 index 0000000..6990144 --- /dev/null +++ b/ReviewSLA/owed.js @@ -0,0 +1,118 @@ +// Who owes a review right now, per assignee (review RFC, Proposal 2). Rebuilt on every run from +// the open PRs' timelines; there's no other state. +// +// An open PR here is clock.js's PR shape plus { repo, url, title, baseRefName, assignees }, and its +// timeline can also hold: +// { type: 'assigned' | 'unassigned' | 'requested', at, login } +// { type: 'pushed', at } a push or force-push to the PR's branch +// { type: 'base', at } the PR's base branch changed (a restack, or the PR below merged) +// +// Each assignee owes a response only on the lowest ready PR in the stack that they're assigned to +// and haven't approved. Their clock there starts at the latest of: the PR being marked ready, their +// assignment, their approval of the PR below, and the PR's base changing (the PR below merging). +// Requesting changes doesn't move them up the stack. Once they've responded, a re-request from the +// author starts a re-review clock, but only if the author has pushed since their last review, so +// Graphite re-requesting on every `gt submit` doesn't. + +const { addBusinessHours, businessHoursBetween } = require('../ReviewConfig/hours.js'); +const { byTime, countsAsResponse, hasSla, isBot, openedAsDraft, targetHours, TARGET_HOURS } = require('./clock.js'); + +// When the PR was last marked ready (or opened, if it was opened ready), or null for a draft. +function readySince(pr) { + if (pr.isDraft) return null; + let start = openedAsDraft(pr) ? null : pr.createdAt; + for (const event of pr.timeline.filter((e) => e.type === 'ready' || e.type === 'draft').sort(byTime)) { + if (event.type === 'ready') start ??= event.at; + else start = null; + } + return start ?? pr.createdAt; +} + +function reviewsBy(pr, login) { + return pr.timeline.filter((e) => e.type === 'review' && e.author?.login === login).sort(byTime); +} + +// The time of `login`'s approval, if their latest approval or changes-requested review is an approval. +function approvedAt(pr, login) { + const decisive = reviewsBy(pr, login).filter((r) => r.state === 'APPROVED' || r.state === 'CHANGES_REQUESTED'); + const last = decisive.at(-1); + return last?.state === 'APPROVED' ? last.at : null; +} + +function latest(pr, type, login) { + return pr.timeline.filter((e) => e.type === type && (login === undefined || e.login === login)).sort(byTime).at(-1)?.at ?? null; +} + +const assigneesOf = (pr) => pr.assignees.filter((login) => login !== pr.author.login && !login.endsWith('[bot]')); + +// The open PRs below `pr` in its stack, nearest first. +function below(pr, byHead) { + const chain = []; + const seen = new Set([pr.number]); + for (let next = byHead.get(pr.baseRefName); next && !seen.has(next.number); next = byHead.get(next.baseRefName)) { + seen.add(next.number); + chain.push(next); + } + return chain; +} + +// Every open PR in `pr`'s stack, including `pr`, bottom first. +function stackOf(pr, prs) { + const byHead = new Map(prs.map((p) => [p.headRefName, p])); + const members = new Map([[pr.number, pr]]); + for (const p of below(pr, byHead)) members.set(p.number, p); + for (let frontier = [pr]; frontier.length > 0; ) { + const next = []; + for (const p of frontier) { + for (const child of prs.filter((c) => c.baseRefName === p.headRefName && !members.has(c.number))) { + members.set(child.number, child); + next.push(child); + } + } + frontier = next; + } + const depth = (p) => below(p, byHead).length; + return [...members.values()].sort((a, b) => depth(a) - depth(b) || a.number - b.number); +} + +function clockFor(pr, login, readyAt, lower) { + // The lowest ready PR they're assigned to and haven't approved is where they owe. + const lowerReady = lower.filter((p) => readySince(p) && assigneesOf(p).includes(login)); + if (lowerReady.some((p) => !approvedAt(p, login))) return null; + + const responses = reviewsBy(pr, login).filter((r) => countsAsResponse(r) && r.at >= readyAt); + if (responses.length === 0) { + const base = latest(pr, 'base'); + const starts = [readyAt, latest(pr, 'assigned', login), base && base > readyAt ? base : null, ...lowerReady.map((p) => approvedAt(p, login))]; + return { kind: 'first', start: new Date(Math.max(...starts.filter(Boolean))), target: targetHours(pr) }; + } + const lastReview = responses.at(-1).at; + const requested = latest(pr, 'requested', login); + const pushed = latest(pr, 'pushed'); + if (requested && requested > lastReview && pushed && pushed > lastReview) { + return { kind: 're-review', start: requested, target: TARGET_HOURS.reReview }; + } + return null; +} + +// Every running clock across `prs` (the open PRs of one repo): +// [{ pr, login, kind: 'first' | 're-review', start, target, due, hours, overdue }]. +function owedClocks(prs, { now, holidays }) { + const byHead = new Map(prs.map((p) => [p.headRefName, p])); + const clocks = []; + for (const pr of prs) { + if (!hasSla(pr) || isBot(pr.author)) continue; + const readyAt = readySince(pr); + if (!readyAt) continue; + const lower = below(pr, byHead); + for (const login of assigneesOf(pr)) { + const clock = clockFor(pr, login, readyAt, lower); + if (!clock || clock.start > now) continue; + const due = addBusinessHours(clock.start, clock.target, holidays); + clocks.push({ pr, login, ...clock, due, hours: businessHoursBetween(clock.start, now, holidays), overdue: now >= due }); + } + } + return clocks; +} + +module.exports = { approvedAt, assigneesOf, owedClocks, readySince, stackOf }; diff --git a/ReviewSLA/owed.test.js b/ReviewSLA/owed.test.js new file mode 100644 index 0000000..6a77cd5 --- /dev/null +++ b/ReviewSLA/owed.test.js @@ -0,0 +1,149 @@ +// Run with: node --test ReviewSLA/ +const test = require('node:test'); +const assert = require('node:assert'); +const { owedClocks, stackOf } = require('./owed.js'); + +const HOLIDAYS = { 2026: [{ date: '2026-11-26' }, { date: '2026-11-27' }] }; +const at = (iso) => new Date(iso); +const user = (login) => ({ login, bot: false }); + +// Thursday Oct 1, 2026, 10:00 Pacific. +const OPENED = '2026-10-01T17:00:00Z'; + +let nextNumber = 1; +function pr({ head, base = 'main', assignees = ['ghopper', 'kjohnson'], timeline = [], ...rest } = {}) { + const number = nextNumber++; + return { + repo: 'StaflLib', + number, + url: `https://github.com/StaflSystems/StaflLib/pull/${number}`, + title: `PR ${number}`, + author: user('alovelace'), + createdAt: at(OPENED), + closedAt: null, + isDraft: false, + additions: 300, + headRefName: head ?? `feature/${number}`, + baseRefName: base, + assignees, + timeline: timeline.map(([type, iso, extra = {}]) => ({ type, at: at(iso), ...extra })), + ...rest, + }; +} +const review = (login, state = 'APPROVED') => ({ author: user(login), state, body: 'ok', comments: 0 }); +const clocks = (prs, now) => owedClocks(prs, { now: at(now), holidays: HOLIDAYS }).map((c) => ({ + pr: c.pr.number, + login: c.login, + kind: c.kind, + start: c.start.toISOString(), + due: c.due.toISOString(), + overdue: c.overdue, +})); + +test('a standard PR opened ready at 10:00 Thursday is due at 17:00 that day, for each assignee', () => { + const p = pr(); + assert.deepStrictEqual(clocks([p], '2026-10-01T23:00:00Z'), [ + { pr: p.number, login: 'ghopper', kind: 'first', start: '2026-10-01T17:00:00.000Z', due: '2026-10-02T00:00:00.000Z', overdue: false }, + { pr: p.number, login: 'kjohnson', kind: 'first', start: '2026-10-01T17:00:00.000Z', due: '2026-10-02T00:00:00.000Z', overdue: false }, + ]); + assert.ok(clocks([p], '2026-10-02T00:00:00Z').every((c) => c.overdue)); +}); + +test('a small PR marked ready at 16:30 Friday is due 13:30 Monday', () => { + const p = pr({ additions: 100, timeline: [['ready', '2026-10-02T23:30:00Z']] }); + assert.strictEqual(clocks([p], '2026-10-05T17:00:00Z')[0].due, '2026-10-05T20:30:00.000Z'); +}); + +test('a review that counts stops only that assignee\'s clock', () => { + const p = pr({ + timeline: [ + ['review', '2026-10-01T18:00:00Z', review('ghopper', 'COMMENTED')], + ['review', '2026-10-01T18:30:00Z', { ...review('kjohnson', 'COMMENTED'), body: '' }], + ['review', '2026-10-01T19:00:00Z', review('mhamilton')], + ], + }); + assert.deepStrictEqual(clocks([p], '2026-10-01T20:00:00Z').map((c) => c.login), ['kjohnson']); +}); + +test('the author, bots and Graphite merge-queue PRs owe nothing, and drafts have no clocks', () => { + assert.deepStrictEqual(clocks([pr({ assignees: ['alovelace', 'renovate[bot]'] })], '2026-10-02T20:00:00Z'), []); + assert.deepStrictEqual(clocks([pr({ author: { login: 'dependabot[bot]', bot: true } })], '2026-10-02T20:00:00Z'), []); + assert.deepStrictEqual(clocks([pr({ head: 'gtmq_spec_1' })], '2026-10-02T20:00:00Z'), []); + assert.deepStrictEqual(clocks([pr({ isDraft: true })], '2026-10-02T20:00:00Z'), []); +}); + +test('going back to draft and ready again starts the clock over, with no miss', () => { + const p = pr({ timeline: [['draft', '2026-10-01T18:00:00Z'], ['ready', '2026-10-05T17:00:00Z']] }); + const [clock] = clocks([p], '2026-10-05T18:00:00Z'); + assert.deepStrictEqual([clock.start, clock.overdue], ['2026-10-05T17:00:00.000Z', false]); +}); + +test('someone assigned later starts their clock when they\'re assigned', () => { + const p = pr({ assignees: ['ghopper'], timeline: [['assigned', '2026-10-02T17:00:00Z', { login: 'ghopper' }]] }); + assert.strictEqual(clocks([p], '2026-10-02T18:00:00Z')[0].start, '2026-10-02T17:00:00.000Z'); +}); + +test('in a three-PR stack with nothing approved, clocks run on the bottom PR only', () => { + const bottom = pr({ head: 'a' }); + const middle = pr({ head: 'b', base: 'a' }); + const top = pr({ head: 'c', base: 'b' }); + assert.deepStrictEqual(new Set(clocks([top, middle, bottom], '2026-10-01T20:00:00Z').map((c) => c.pr)), new Set([bottom.number])); + assert.deepStrictEqual(stackOf(middle, [top, middle, bottom]).map((p) => p.number), [bottom.number, middle.number, top.number]); +}); + +test('approving the bottom PR moves that assignee up the stack, from the time they approved', () => { + const bottom = pr({ head: 'a', timeline: [['review', '2026-10-01T20:00:00Z', review('ghopper')]] }); + const middle = pr({ head: 'b', base: 'a' }); + const top = pr({ head: 'c', base: 'b' }); + assert.deepStrictEqual( + clocks([bottom, middle, top], '2026-10-01T21:00:00Z').map((c) => [c.pr, c.login, c.start]), + [ + [bottom.number, 'kjohnson', '2026-10-01T17:00:00.000Z'], + [middle.number, 'ghopper', '2026-10-01T20:00:00.000Z'], + ], + ); +}); + +test('requesting changes stops the reviewer\'s clock and starts nothing higher up', () => { + const bottom = pr({ head: 'a', assignees: ['ghopper'], timeline: [['review', '2026-10-01T20:00:00Z', review('ghopper', 'CHANGES_REQUESTED')]] }); + const top = pr({ head: 'b', base: 'a', assignees: ['ghopper'] }); + assert.deepStrictEqual(clocks([bottom, top], '2026-10-02T21:00:00Z'), []); +}); + +test('a draft PR below doesn\'t hold the clock back', () => { + const bottom = pr({ head: 'a', isDraft: true }); + const top = pr({ head: 'b', base: 'a' }); + assert.deepStrictEqual(new Set(clocks([bottom, top], '2026-10-01T20:00:00Z').map((c) => c.pr)), new Set([top.number])); +}); + +test('when the PR below merges, the clock starts when the base changes', () => { + const p = pr({ assignees: ['ghopper'], timeline: [['base', '2026-10-02T18:00:00Z']] }); + assert.strictEqual(clocks([p], '2026-10-02T19:00:00Z')[0].start, '2026-10-02T18:00:00.000Z'); +}); + +test('the author pushing a fix and re-requesting review starts a 4-hour re-review clock', () => { + const p = pr({ + assignees: ['ghopper'], + timeline: [ + ['review', '2026-10-01T18:00:00Z', review('ghopper', 'CHANGES_REQUESTED')], + ['pushed', '2026-10-01T20:00:00Z'], + ['requested', '2026-10-01T20:05:00Z', { login: 'ghopper' }], + ], + }); + assert.deepStrictEqual( + clocks([p], '2026-10-01T21:00:00Z').map((c) => [c.kind, c.start, c.due]), + [['re-review', '2026-10-01T20:05:00.000Z', '2026-10-02T17:05:00.000Z']], + ); +}); + +test('a re-request with no push since the last review (a Graphite resubmit) starts nothing', () => { + const p = pr({ + assignees: ['ghopper'], + timeline: [ + ['pushed', '2026-10-01T17:30:00Z'], + ['review', '2026-10-01T18:00:00Z', review('ghopper', 'COMMENTED')], + ['requested', '2026-10-01T20:05:00Z', { login: 'ghopper' }], + ], + }); + assert.deepStrictEqual(clocks([p], '2026-10-01T21:00:00Z'), []); +}); diff --git a/ReviewSLA/run.js b/ReviewSLA/run.js new file mode 100644 index 0000000..55cd72a --- /dev/null +++ b/ReviewSLA/run.js @@ -0,0 +1,235 @@ +// The scheduled Review SLA job (review RFC, Proposal 2). Each run rebuilds every assignee's clock +// from the open PRs' timelines, then, depending on REVIEW_SLA_MODE: +// +// off does nothing +// shadow keeps each PR's Review SLA comment, the Confluence dashboard and the 10:00 Slack digest +// up to date (kept as the rollback) +// remind also DMs each assignee when their review is due, and reassigns an assignee the PTO +// calendar has out before their review is due, across the whole stack +// enforce reassignment at twice the target (step 3); not built yet, so it runs as remind +// +// It fails safe: an API error is a warning, and that PR, repo or channel is skipped for this run. +// A missed reminder is cheap; a wrong reassignment isn't. + +const { addDays, pacificDate, readHolidays, readOutDates } = require('../ReviewConfig/pto.js'); +const { isBusinessDay, pacificOffset, pacificTime } = require('../ReviewConfig/hours.js'); +const { pickLeastLoaded, teamMembers } = require('../ReviewConfig/pick.js'); +const { hasSla } = require('./clock.js'); +const { parseState, reminderKey, renderComment } = require('./comment.js'); +const { renderDashboard, updateDashboard } = require('./confluence.js'); +const { fetchOpenPrs } = require('./github.js'); +const { assigneesOf, owedClocks, readySince, stackOf } = require('./owed.js'); +const { digestText, postMessage, reminderText } = require('./slack.js'); + +const MODES = ['off', 'shadow', 'remind', 'enforce']; + +// The scheduled runs at 10:00 Pacific, which post the digest: 17:00 UTC in daylight time and 18:00 +// in standard time. Both fire all year; only the one that is 10:00 Pacific that day posts it. +const DIGEST_CRONS = { '-7': '0 17 * * 1-5', '-8': '0 18 * * 1-5' }; + +function readMode(value, core) { + const mode = (value || 'off').trim().toLowerCase(); + if (!MODES.includes(mode)) { + core.warning(`Unknown REVIEW_SLA_MODE '${value}'; running as shadow.`); + return 'shadow'; + } + if (mode === 'enforce') { + core.warning("REVIEW_SLA_MODE 'enforce' (reassignment at twice the target) isn't built yet; running as remind."); + return 'remind'; + } + return mode; +} + +function readPeople(value, core) { + if (value && typeof value === 'object') return value; + try { + return JSON.parse(value || '{}'); + } catch { + core.warning("The REVIEW_PEOPLE org variable isn't valid JSON; reminders and names are off this run."); + return {}; + } +} + +// Business days, from opening until half an hour after closing, so dues at 17:00 are caught. +function inBusinessHours(now, holidays) { + const today = pacificDate(now); + return isBusinessDay(today, holidays) && now >= pacificTime(today, 10) && now <= pacificTime(today, 17.5); +} + +function isDigestRun(inputs, now) { + if (String(inputs.digest) === 'true') return true; + return inputs.eventName === 'schedule' && inputs.schedule === DIGEST_CRONS[String(pacificOffset(pacificDate(now)))]; +} + +// Whether the calendar has `login` out on any day from today to the day the review is due. +function outBefore(out, login, due, now) { + const dates = out[login]; + if (!dates) return false; + for (let date = pacificDate(now); date <= pacificDate(due); date = addDays(date, 1)) if (dates.has(date)) return true; + return false; +} + +// Replaces `login` with someone from the same team on every PR in the stack they're assigned to. +// The domain approver is replaced from the domain team, the rotation reviewer from the rotation team. +async function reassign({ github, core, owner, inputs, teams, clock, stack, out, now }) { + const { pr, login } = clock; + const others = assigneesOf(pr).filter((l) => l !== login); + const domain = await teams(inputs.domainTeam); + const fromDomain = domain.includes(login) && !others.some((l) => domain.includes(l)); + const team = fromDomain ? inputs.domainTeam : inputs.rotationTeam; + const members = fromDomain ? domain : await teams(inputs.rotationTeam); + const authors = new Set(stack.map((p) => p.author.login)); + const candidates = members.filter((m) => m !== login && !others.includes(m) && !authors.has(m)); + const away = new Set(Object.keys(out).filter((l) => outBefore(out, l, clock.due, now))); + const chosen = await pickLeastLoaded({ github, org: owner, core, candidates, away, seed: pr.number }); + if (!chosen || away.has(chosen.login)) { + core.warning(`${pr.repo}#${pr.number}: ${login} is out before their review is due, but no one else in ${team} is available.`); + return []; + } + const changed = stack.filter((p) => assigneesOf(p).includes(login)); + for (const p of changed) { + await github.rest.issues.removeAssignees({ owner, repo: p.repo, issue_number: p.number, assignees: [login] }); + await github.rest.issues.addAssignees({ owner, repo: p.repo, issue_number: p.number, assignees: [chosen.login] }); + p.assignees = [...p.assignees.filter((l) => l !== login), chosen.login]; + try { + await github.rest.pulls.requestReviewers({ owner, repo: p.repo, pull_number: p.number, reviewers: [chosen.login] }); + } catch (error) { + core.warning(`${p.repo}#${p.number}: couldn't request a review from ${chosen.login} (${error.message}).`); + } + } + const numbers = changed.map((p) => `#${p.number}`).join(', '); + await github.rest.issues.createComment({ + owner, + repo: pr.repo, + issue_number: changed[0].number, + body: + `**Review SLA:** the PTO calendar has \`${login}\` out before their review is due, so @${chosen.login} ` + + `takes over from them as assignee on ${numbers}, picked from ${team} by fewest open assigned PRs.`, + }); + core.info(`${pr.repo}: reassigned ${login} to ${chosen.login} on ${numbers} (out of office).`); + return changed.map((p) => `${p.repo}#${p.number}:${login}`); +} + +module.exports = async function run({ github, core, inputs, now = new Date(), fetch = globalThis.fetch, holidays = readHolidays() }) { + const mode = readMode(inputs.mode, core); + if (mode === 'off') return core.info('REVIEW_SLA_MODE is off: nothing to do.'); + if (inputs.eventName === 'schedule' && !inBusinessHours(now, holidays)) { + return core.info('Outside business hours: nothing to do.'); + } + const remind = mode === 'remind'; + const owner = inputs.owner || 'StaflSystems'; + const people = readPeople(inputs.people, core); + const repos = (inputs.repos || '').split(/[\s,]+/).filter(Boolean); + const dashboardUrl = inputs.dashboardUrl || ''; + + // 1. Every open PR, and every running clock. + const prs = []; + let clocks = []; + for (const repo of repos) { + try { + const open = await fetchOpenPrs({ github, owner, repo }); + prs.push(...open); + clocks.push(...owedClocks(open, { now, holidays })); + } catch (error) { + core.warning(`${repo}: couldn't read open PRs (${error.message}); skipping it this run.`); + } + } + + // 2. Out of office: hand the review to someone else before it's due. + if (remind && clocks.length > 0) { + const out = await readOutDates({ url: inputs.ptoCalendarUrl, core, people, now, fetch, holidays }); + if (out) { + const teamCache = new Map(); + const teams = async (slug) => { + if (!teamCache.has(slug)) teamCache.set(slug, await teamMembers(github, owner, slug)); + return teamCache.get(slug); + }; + const reassigned = new Set(); + for (const clock of clocks) { + if (reassigned.has(`${clock.pr.repo}#${clock.pr.number}:${clock.login}`) || !outBefore(out, clock.login, clock.due, now)) continue; + const stack = stackOf(clock.pr, prs.filter((p) => p.repo === clock.pr.repo)); + try { + for (const key of await reassign({ github, core, owner, inputs, teams, clock, stack, out, now })) reassigned.add(key); + } catch (error) { + core.warning(`${clock.pr.repo}#${clock.pr.number}: couldn't reassign ${clock.login} (${error.message}).`); + } + } + clocks = clocks.filter((c) => !reassigned.has(`${c.pr.repo}#${c.pr.number}:${c.login}`)); + } + } + + // 3. Each PR's comment, then the DMs it records. The comment is written first, so a failed + // write means a missed reminder rather than a repeated one. + const slackIdMissing = new Set(); + let reminders = 0; + for (const pr of prs) { + if (!hasSla(pr)) continue; + const mine = clocks.filter((c) => c.pr === pr); + if (!pr.sticky && mine.length === 0) continue; + const keys = new Set(mine.map(reminderKey)); + const reminded = parseState(pr.sticky?.body).reminded.filter((k) => keys.has(k)); + const due = !remind || !inputs.slackToken ? [] : mine.filter((c) => c.overdue && !reminded.includes(reminderKey(c))); + const sendable = due.filter((c) => { + if (people[c.login]?.slack) return true; + slackIdMissing.add(c.login); + return false; + }); + const state = { reminded: [...reminded, ...sendable.map(reminderKey)] }; + const body = renderComment(readySince(pr) ? mine : [], state, { dashboardUrl }); + try { + if (!pr.sticky) await github.rest.issues.createComment({ owner, repo: pr.repo, issue_number: pr.number, body }); + else if (body !== pr.sticky.body) await github.rest.issues.updateComment({ owner, repo: pr.repo, comment_id: pr.sticky.id, body }); + } catch (error) { + core.warning(`${pr.repo}#${pr.number}: couldn't write the Review SLA comment (${error.message}); no reminders for it this run.`); + continue; + } + for (const clock of sendable) { + try { + await postMessage({ token: inputs.slackToken, channel: people[clock.login].slack, text: reminderText(clock), fetch }); + reminders++; + } catch (error) { + core.warning(`${pr.repo}#${pr.number}: couldn't DM ${clock.login} (${error.message}).`); + } + } + } + if (slackIdMissing.size > 0) core.warning(`No Slack ID in REVIEW_PEOPLE for ${[...slackIdMissing].join(', ')}, so they get no reminders.`); + + // 4. The 10:00 digest. + if (isDigestRun(inputs, now)) { + if (!inputs.slackToken || !inputs.slackChannel) core.info('No Slack token or digest channel configured; no digest.'); + else { + try { + await postMessage({ token: inputs.slackToken, channel: inputs.slackChannel, text: digestText(clocks, { now, people, dashboardUrl }), fetch }); + core.info('Posted the digest.'); + } catch (error) { + core.warning(`Couldn't post the digest (${error.message}).`); + } + } + } + + // 5. The dashboard. + if (!inputs.confluenceUrl || !inputs.confluenceUser || !inputs.confluenceToken || !inputs.confluencePageId) { + core.info('Confluence isn\'t fully configured; not updating the dashboard.'); + } else { + try { + const saved = await updateDashboard({ + baseUrl: inputs.confluenceUrl, + user: inputs.confluenceUser, + token: inputs.confluenceToken, + pageId: inputs.confluencePageId, + body: renderDashboard(clocks, { people }), + fetch, + }); + core.info(saved ? 'Updated the dashboard.' : 'Dashboard unchanged.'); + } catch (error) { + core.warning(`Couldn't update the dashboard (${error.message}).`); + } + } + + const overdue = clocks.filter((c) => c.overdue).length; + core.info(`Mode ${mode}: ${prs.length} open PRs, ${clocks.length} reviews owed, ${overdue} overdue, ${reminders} reminders sent.`); + return { clocks, reminders }; +}; + +module.exports.inBusinessHours = inBusinessHours; +module.exports.isDigestRun = isDigestRun; diff --git a/ReviewSLA/run.test.js b/ReviewSLA/run.test.js new file mode 100644 index 0000000..5b9b038 --- /dev/null +++ b/ReviewSLA/run.test.js @@ -0,0 +1,251 @@ +// Run with: node --test ReviewSLA/ +const test = require('node:test'); +const assert = require('node:assert'); +const run = require('./run.js'); +const { MARKER } = require('./comment.js'); + +const STAFF = ['ghopper', 'mhamilton']; +const EVERYONE = [...STAFF, 'kjohnson', 'dvaughan', 'alovelace']; +const PEOPLE = Object.fromEntries(EVERYONE.map((login, i) => [login, { name: `Person ${login}`, slack: `U00${i}` }])); +const HOLIDAYS = { 2026: [{ date: '2026-11-26' }, { date: '2026-11-27' }] }; +const PTO_URL = 'https://calendar.example/pto.ics'; + +// Thursday Oct 1, 2026, 10:00 Pacific. +const OPENED = '2026-10-01T17:00:00Z'; + +const userNode = (login) => ({ __typename: 'User', login }); +const reviewItem = (login, state, iso) => ({ __typename: 'PullRequestReview', state, submittedAt: iso, body: 'ok', comments: { totalCount: 0 }, author: userNode(login) }); + +// A fake org: open PRs (as GraphQL nodes, rebuilt on every query so comments written by one run +// are read by the next), teams, Slack, Confluence and the PTO calendar. +function world({ prs, out = [], failRepo, failComments = false }) { + const calls = { dms: [], posts: [], comments: [], edits: [], assign: [], unassign: [], requested: [], puts: [], graphql: 0 }; + const state = new Map(prs.map((p) => [p.number, { repo: 'StaflLib', author: 'alovelace', base: 'main', isDraft: false, additions: 300, timeline: [], assignees: [], comments: [], ...p }])); + let commentId = 100; + let page = { title: 'Review Dashboard', version: { number: 3, message: '' } }; + const asNode = (p) => ({ + number: p.number, + url: `https://github.com/StaflSystems/${p.repo}/pull/${p.number}`, + title: `PR ${p.number}`, + isDraft: p.isDraft, + additions: p.additions, + headRefName: p.head, + baseRefName: p.base, + createdAt: OPENED, + author: userNode(p.author), + assignees: { nodes: p.assignees.map((login) => ({ login })) }, + comments: { nodes: p.comments }, + timelineItems: { pageInfo: { hasPreviousPage: false }, nodes: p.timeline }, + }); + const github = { + paginate: async (fn, params) => (await fn(params)).data, + graphql: async (query, vars) => { + calls.graphql++; + if (query.includes('organization(login')) { + return { organization: { repositories: { nodes: [{ name: 'StaflLib', pullRequests: { nodes: [] } }] } } }; + } + if (vars.name === failRepo) throw new Error('HTTP 502'); + const nodes = [...state.values()].filter((p) => p.repo === vars.name).map(asNode); + return { repository: { pullRequests: { pageInfo: { hasNextPage: false }, nodes } } }; + }, + rest: { + teams: { + listMembersInOrg: async ({ team_slug }) => ({ data: (team_slug === 'embeddedreviewersstaff' ? STAFF : EVERYONE).map((login) => ({ login })) }), + }, + issues: { + createComment: async ({ issue_number, body }) => { + if (failComments) throw new Error('HTTP 500'); + calls.comments.push({ issue_number, body }); + state.get(issue_number).comments.push({ databaseId: commentId++, body }); + }, + updateComment: async ({ comment_id, body }) => { + calls.edits.push({ comment_id, body }); + for (const p of state.values()) for (const c of p.comments) if (c.databaseId === comment_id) c.body = body; + }, + addAssignees: async ({ issue_number, assignees }) => { + calls.assign.push([issue_number, ...assignees]); + state.get(issue_number).assignees.push(...assignees); + }, + removeAssignees: async ({ issue_number, assignees }) => { + calls.unassign.push([issue_number, ...assignees]); + const p = state.get(issue_number); + p.assignees = p.assignees.filter((a) => !assignees.includes(a)); + }, + }, + pulls: { + requestReviewers: async ({ pull_number, reviewers }) => calls.requested.push([pull_number, ...reviewers]), + }, + }, + }; + const fetch = async (url, options = {}) => { + if (url === PTO_URL) { + const day = (d) => d.replaceAll('-', ''); + const events = out.map(([name, date]) => `BEGIN:VEVENT\r\nDTSTART;VALUE=DATE:${day(date)}\r\nSUMMARY:${name} is Out of Office\r\nEND:VEVENT`); + return { ok: true, status: 200, text: async () => ['BEGIN:VCALENDAR', ...events, 'END:VCALENDAR'].join('\r\n') }; + } + if (url.startsWith('https://slack.com/')) { + const message = JSON.parse(options.body); + (message.channel.startsWith('U') ? calls.dms : calls.posts).push(message); + return { ok: true, status: 200, json: async () => ({ ok: true }) }; + } + if (url.includes('/rest/api/content/')) { + if (options.method === 'PUT') { + const update = JSON.parse(options.body); + calls.puts.push(update); + page = { title: update.title, version: update.version }; + return { ok: true, status: 200, json: async () => ({}) }; + } + return { ok: true, status: 200, json: async () => page }; + } + throw new Error(`unexpected fetch ${url}`); + }; + return { github, fetch, calls, pr: (n) => state.get(n) }; +} + +function logger() { + const logs = []; + return { logs, core: { info: (m) => logs.push(m), warning: (m) => logs.push(`WARN ${m}`) } }; +} + +const INPUTS = { + repos: 'StaflLib', + rotationTeam: 'embeddedreviewers', + domainTeam: 'embeddedreviewersstaff', + people: JSON.stringify(PEOPLE), + ptoCalendarUrl: PTO_URL, + slackToken: 'xoxb-test', + slackChannel: 'C123', + confluenceUrl: 'https://example.atlassian.net/wiki', + confluenceUser: 'sla@example.com', + confluenceToken: 'token', + confluencePageId: '42', + dashboardUrl: 'https://example.atlassian.net/wiki/spaces/EM/pages/42', + eventName: 'schedule', + schedule: '*/30 17-23 * * 1-5', +}; + +async function go(w, { mode = 'remind', now, inputs = {} } = {}) { + const { core, logs } = logger(); + const result = await run({ github: w.github, core, fetch: w.fetch, holidays: HOLIDAYS, now: new Date(now), inputs: { ...INPUTS, mode, ...inputs } }); + return { logs, result }; +} + +// One standard PR opened ready at 10:00 Thursday, due at 17:00. +const onePr = (extra = {}) => ({ number: 1, head: 'feature/a', assignees: ['ghopper', 'kjohnson'], ...extra }); + +test('off does nothing at all', async () => { + const w = world({ prs: [onePr()] }); + const { logs } = await go(w, { mode: 'off', now: '2026-10-02T00:00:00Z' }); + assert.strictEqual(w.calls.graphql, 0); + assert.deepStrictEqual(logs, ['REVIEW_SLA_MODE is off: nothing to do.']); +}); + +test('scheduled runs outside business hours do nothing', async () => { + const w = world({ prs: [onePr()] }); + await go(w, { now: '2026-10-03T19:00:00Z' }); // Saturday + await go(w, { now: '2026-10-01T15:00:00Z' }); // 08:00 Thursday + assert.strictEqual(w.calls.graphql, 0); +}); + +test('shadow keeps the comment and dashboard but sends no reminders and reassigns nobody', async () => { + const w = world({ prs: [onePr()], out: [['Person ghopper', '2026-10-02']] }); + await go(w, { mode: 'shadow', now: '2026-10-02T00:00:00Z' }); + assert.strictEqual(w.calls.comments.length, 1); + assert.ok(w.calls.comments[0].body.startsWith(`${MARKER} {"reminded":[]} -->`)); + assert.match(w.calls.comments[0].body, /\| `ghopper` \| First response \| Thu Oct 1, 17:00 PT \| Overdue \|/); + assert.deepStrictEqual([w.calls.dms, w.calls.assign], [[], []]); + assert.strictEqual(w.calls.puts.length, 1); +}); + +test('remind DMs each overdue assignee once, however many runs see it', async () => { + const w = world({ prs: [onePr()] }); + await go(w, { now: '2026-10-01T23:30:00Z' }); + assert.deepStrictEqual(w.calls.dms, []); + await go(w, { now: '2026-10-02T00:00:00Z' }); + assert.deepStrictEqual(w.calls.dms.map((m) => m.channel).sort(), ['U000', 'U002']); + assert.match(w.calls.dms[0].text, /^Review due: \. As an assignee, your first response was due at Thu Oct 1, 17:00 PT\.$/); + await go(w, { now: '2026-10-02T17:30:00Z' }); + assert.strictEqual(w.calls.dms.length, 2); + // One comment, edited in place. + assert.strictEqual(w.calls.comments.length, 1); +}); + +test('no comment is created for a PR nobody owes a review on', async () => { + const w = world({ prs: [onePr({ timeline: [reviewItem('ghopper', 'APPROVED', '2026-10-01T18:00:00Z'), reviewItem('kjohnson', 'APPROVED', '2026-10-01T18:00:00Z')] })] }); + await go(w, { now: '2026-10-02T00:00:00Z' }); + assert.deepStrictEqual(w.calls.comments, []); +}); + +test('an assignee out before their review is due is replaced across the stack, and only them', async () => { + // Bottom PR #1 and top PR #2 both have ghopper (domain approver) and kjohnson. + const w = world({ + prs: [onePr(), { number: 2, head: 'feature/b', base: 'feature/a', assignees: ['ghopper', 'kjohnson'] }], + out: [['Person ghopper', '2026-10-01']], + }); + await go(w, { now: '2026-10-01T18:00:00Z' }); + assert.deepStrictEqual(w.calls.unassign, [[1, 'ghopper'], [2, 'ghopper']]); + assert.deepStrictEqual(w.calls.assign, [[1, 'mhamilton'], [2, 'mhamilton']]); + assert.deepStrictEqual(w.pr(1).assignees.sort(), ['kjohnson', 'mhamilton']); + const notice = w.calls.comments.find((c) => !c.body.startsWith(MARKER)); + assert.strictEqual(notice.issue_number, 1); + assert.match(notice.body, /`ghopper` out before their review is due, so @mhamilton takes over from them as assignee on #1, #2/); +}); + +test('someone out after their review is due is left alone', async () => { + const w = world({ prs: [onePr()], out: [['Person ghopper', '2026-10-02']] }); + await go(w, { now: '2026-10-01T18:00:00Z' }); + assert.deepStrictEqual(w.calls.assign, []); +}); + +test('the digest posts only on the 10:00 Pacific run', async () => { + const w = world({ prs: [onePr()] }); + await go(w, { now: '2026-10-02T17:05:00Z', inputs: { schedule: '0 18 * * 1-5' } }); + assert.deepStrictEqual(w.calls.posts, []); + await go(w, { now: '2026-10-02T17:05:00Z', inputs: { schedule: '0 17 * * 1-5' } }); + assert.strictEqual(w.calls.posts.length, 1); + assert.strictEqual(w.calls.posts[0].channel, 'C123'); + assert.match(w.calls.posts[0].text, /^\*Review SLA, Fri Oct 2\*\n2 overdue reviews:\n• Person ghopper: /); + assert.ok(run.isDigestRun({ eventName: 'schedule', schedule: '0 18 * * 1-5' }, new Date('2026-12-01T18:00:00Z'))); + assert.ok(run.isDigestRun({ digest: 'true', eventName: 'workflow_dispatch' }, new Date('2026-12-01T20:00:00Z'))); +}); + +test('the dashboard saves a new version only when its content changes, as a minor edit', async () => { + const w = world({ prs: [onePr()] }); + await go(w, { now: '2026-10-01T18:00:00Z' }); + await go(w, { now: '2026-10-01T18:30:00Z' }); + assert.strictEqual(w.calls.puts.length, 1); + assert.deepStrictEqual([w.calls.puts[0].version.number, w.calls.puts[0].version.minorEdit], [4, true]); + await go(w, { now: '2026-10-02T00:00:00Z' }); + assert.strictEqual(w.calls.puts.length, 2); + assert.match(w.calls.puts[1].body.storage.value, /Person ghopper<\/td>1 overdue<\/td>/); + assert.match(w.calls.puts[1].body.storage.value, /Person dvaughan<\/td>✅<\/td>/); +}); + +test('a repo that can\'t be read is a warning, and the other repos carry on', async () => { + const w = world({ prs: [onePr()], failRepo: 'coit-tower-bms2000' }); + const { logs } = await go(w, { now: '2026-10-02T00:00:00Z', inputs: { repos: 'coit-tower-bms2000 StaflLib' } }); + assert.ok(logs.includes("WARN coit-tower-bms2000: couldn't read open PRs (HTTP 502); skipping it this run.")); + assert.strictEqual(w.calls.dms.length, 2); +}); + +test('if the comment can\'t be written, no reminder is sent for that PR', async () => { + const w = world({ prs: [onePr()], failComments: true }); + const { logs } = await go(w, { now: '2026-10-02T00:00:00Z' }); + assert.deepStrictEqual(w.calls.dms, []); + assert.ok(logs.some((l) => l.startsWith("WARN StaflLib#1: couldn't write the Review SLA comment"))); +}); + +test('someone with no Slack ID is warned about, and the others still get reminders', async () => { + const people = { ...PEOPLE, ghopper: { name: 'Person ghopper' } }; + const w = world({ prs: [onePr()] }); + const { logs } = await go(w, { now: '2026-10-02T00:00:00Z', inputs: { people: JSON.stringify(people) } }); + assert.deepStrictEqual(w.calls.dms.map((m) => m.channel), ['U002']); + assert.ok(logs.includes('WARN No Slack ID in REVIEW_PEOPLE for ghopper, so they get no reminders.')); +}); + +test('enforce isn\'t built yet and runs as remind', async () => { + const w = world({ prs: [onePr()] }); + const { logs } = await go(w, { mode: 'enforce', now: '2026-10-02T00:00:00Z' }); + assert.ok(logs.some((l) => l.startsWith("WARN REVIEW_SLA_MODE 'enforce'"))); + assert.strictEqual(w.calls.dms.length, 2); +}); diff --git a/ReviewSLA/slack.js b/ReviewSLA/slack.js new file mode 100644 index 0000000..2777de1 --- /dev/null +++ b/ReviewSLA/slack.js @@ -0,0 +1,41 @@ +// Slack: reminder DMs to assignees, and the 10:00 digest. One bot token does both; its only scope +// is chat:write, and the bot has to be invited to the digest channel. + +const { pacificDate } = require('../ReviewConfig/pto.js'); +const { formatTime, KIND } = require('./comment.js'); + +async function postMessage({ token, channel, text, fetch = globalThis.fetch }) { + const response = await fetch('https://slack.com/api/chat.postMessage', { + method: 'POST', + headers: { authorization: `Bearer ${token}`, 'content-type': 'application/json; charset=utf-8' }, + body: JSON.stringify({ channel, text, unfurl_links: false, unfurl_media: false }), + }); + const body = await response.json().catch(() => ({})); + if (!response.ok || !body.ok) throw new Error(`Slack: ${body.error ?? `HTTP ${response.status}`}`); +} + +// Slack's mrkdwn wants &, < and > escaped in text. +const escape = (text) => text.replace(/&/g, '&').replace(//g, '>'); +const prLink = (pr) => `<${pr.url}|${pr.repo}#${pr.number}: ${escape(pr.title)}>`; + +function reminderText(clock) { + const what = clock.kind === 'first' ? 'your first response' : 'your re-review'; + return `Review due: ${prLink(clock.pr)}. As an assignee, ${what} was due at ${formatTime(clock.due)} PT.`; +} + +function digestText(clocks, { now, people, dashboardUrl }) { + const nameOf = (login) => people[login]?.name || login; + const overdue = clocks.filter((c) => c.overdue).sort((a, b) => nameOf(a.login).localeCompare(nameOf(b.login)) || a.due - b.due); + const dueToday = clocks.filter((c) => !c.overdue && pacificDate(c.due) === pacificDate(now)); + const lines = [`*Review SLA, ${formatTime(now).split(',')[0]}*`]; + if (overdue.length === 0) lines.push('No overdue reviews.'); + else { + lines.push(`${overdue.length} overdue ${overdue.length === 1 ? 'review' : 'reviews'}:`); + for (const c of overdue) lines.push(`• ${escape(nameOf(c.login))}: ${prLink(c.pr)} (${KIND[c.kind].toLowerCase()}, due ${formatTime(c.due)})`); + } + if (dueToday.length > 0) lines.push(`${dueToday.length} more due today.`); + if (dashboardUrl) lines.push(`<${dashboardUrl}|Dashboard>`); + return lines.join('\n'); +} + +module.exports = { digestText, postMessage, reminderText }; From 93b1b3d1ff53f92509781ec96941c0dc0b28a010 Mon Sep 17 00:00:00 2001 From: Thomas Willson Date: Thu, 1 Oct 2026 17:08:05 -0700 Subject: [PATCH 2/5] ReviewSLA: tell a push since the last review by commit, not commit dates Commit dates need the app's Contents permission in private repos, and a rebase rewrites them. The PR's head commit against the commit each review was left on needs only Pull requests read, and can't be fooled by a rebase. Co-Authored-By: Claude Opus 5.5 --- ReviewSLA/README.md | 6 +++--- ReviewSLA/github.js | 14 ++++---------- ReviewSLA/owed.js | 15 ++++++++------- ReviewSLA/owed.test.js | 8 ++++---- 4 files changed, 19 insertions(+), 24 deletions(-) diff --git a/ReviewSLA/README.md b/ReviewSLA/README.md index 3c0a071..96a54e3 100644 --- a/ReviewSLA/README.md +++ b/ReviewSLA/README.md @@ -28,9 +28,9 @@ Each assignee other than the author has their own clock on each ready PR: or at least one inline comment. - **Stacks:** an assignee owes a response only on the lowest ready PR in the stack they're assigned to and haven't approved. Requesting changes doesn't move them up the stack. -- **Re-review:** due 4 business hours after the author re-requests their review, if the author - has pushed since their last one. A re-request with no new push, as Graphite makes on every - `gt submit`, starts nothing. +- **Re-review:** due 4 business hours after the author re-requests their review, if the PR's head + commit has changed since their last one. A re-request with no new push, as Graphite makes on + every `gt submit`, starts nothing. - **Drafts** stop every clock on the PR. Marking it ready again starts the first-response clocks over. diff --git a/ReviewSLA/github.js b/ReviewSLA/github.js index 4417809..6131d02 100644 --- a/ReviewSLA/github.js +++ b/ReviewSLA/github.js @@ -9,8 +9,6 @@ const TIMELINE_TYPES = [ 'ASSIGNED_EVENT', 'UNASSIGNED_EVENT', 'REVIEW_REQUESTED_EVENT', - 'HEAD_REF_FORCE_PUSHED_EVENT', - 'PULL_REQUEST_COMMIT', 'BASE_REF_CHANGED_EVENT', ].join(', '); @@ -20,12 +18,10 @@ const TIMELINE_FIELDS = ` __typename ... on ReadyForReviewEvent { createdAt } ... on ConvertToDraftEvent { createdAt } - ... on PullRequestReview { state submittedAt body comments { totalCount } author { __typename login } } + ... on PullRequestReview { state submittedAt body comments { totalCount } author { __typename login } commit { oid } } ... on AssignedEvent { createdAt assignee { ... on User { login } } } ... on UnassignedEvent { createdAt assignee { ... on User { login } } } ... on ReviewRequestedEvent { createdAt requestedReviewer { ... on User { login } } } - ... on HeadRefForcePushedEvent { createdAt } - ... on PullRequestCommit { commit { committedDate } } ... on BaseRefChangedEvent { createdAt } }`; @@ -34,7 +30,7 @@ const OPEN_PRS = `query($owner: String!, $name: String!, $after: String) { pullRequests(states: OPEN, first: 30, after: $after) { pageInfo { hasNextPage endCursor } nodes { - number url title isDraft additions headRefName baseRefName createdAt + number url title isDraft additions headRefName headRefOid baseRefName createdAt author { __typename login } assignees(first: 10) { nodes { login } } comments(last: 50) { nodes { databaseId body } } @@ -70,16 +66,13 @@ function toEvent(item) { state: item.state, body: item.body, comments: item.comments.totalCount, + commit: item.commit?.oid ?? null, }; case 'AssignedEvent': case 'UnassignedEvent': return item.assignee?.login && { type: item.__typename === 'AssignedEvent' ? 'assigned' : 'unassigned', at: at(item.createdAt), login: item.assignee.login }; case 'ReviewRequestedEvent': return item.requestedReviewer?.login && { type: 'requested', at: at(item.createdAt), login: item.requestedReviewer.login }; - case 'HeadRefForcePushedEvent': - return { type: 'pushed', at: at(item.createdAt) }; - case 'PullRequestCommit': - return item.commit?.committedDate && { type: 'pushed', at: at(item.commit.committedDate) }; case 'BaseRefChangedEvent': return { type: 'base', at: at(item.createdAt) }; default: @@ -97,6 +90,7 @@ function toOpenPr(repo, node, items) { isDraft: node.isDraft, additions: node.additions, headRefName: node.headRefName, + headRefOid: node.headRefOid, baseRefName: node.baseRefName, createdAt: new Date(node.createdAt), closedAt: null, diff --git a/ReviewSLA/owed.js b/ReviewSLA/owed.js index 6990144..abedc6a 100644 --- a/ReviewSLA/owed.js +++ b/ReviewSLA/owed.js @@ -1,18 +1,19 @@ // Who owes a review right now, per assignee (review RFC, Proposal 2). Rebuilt on every run from // the open PRs' timelines; there's no other state. // -// An open PR here is clock.js's PR shape plus { repo, url, title, baseRefName, assignees }, and its +// An open PR here is clock.js's PR shape plus { repo, url, title, baseRefName, headRefOid, +// assignees }. Its reviews also carry `commit`, the head commit they were left on, and its // timeline can also hold: // { type: 'assigned' | 'unassigned' | 'requested', at, login } -// { type: 'pushed', at } a push or force-push to the PR's branch // { type: 'base', at } the PR's base branch changed (a restack, or the PR below merged) // // Each assignee owes a response only on the lowest ready PR in the stack that they're assigned to // and haven't approved. Their clock there starts at the latest of: the PR being marked ready, their // assignment, their approval of the PR below, and the PR's base changing (the PR below merging). // Requesting changes doesn't move them up the stack. Once they've responded, a re-request from the -// author starts a re-review clock, but only if the author has pushed since their last review, so -// Graphite re-requesting on every `gt submit` doesn't. +// author starts a re-review clock, but only if the PR's head has moved since their last review, so +// Graphite re-requesting on every `gt submit` doesn't. (Comparing commits rather than push times +// also means the app needs no Contents permission, and a rebase can't fake a date.) const { addBusinessHours, businessHoursBetween } = require('../ReviewConfig/hours.js'); const { byTime, countsAsResponse, hasSla, isBot, openedAsDraft, targetHours, TARGET_HOURS } = require('./clock.js'); @@ -86,10 +87,10 @@ function clockFor(pr, login, readyAt, lower) { const starts = [readyAt, latest(pr, 'assigned', login), base && base > readyAt ? base : null, ...lowerReady.map((p) => approvedAt(p, login))]; return { kind: 'first', start: new Date(Math.max(...starts.filter(Boolean))), target: targetHours(pr) }; } - const lastReview = responses.at(-1).at; + const lastReview = responses.at(-1); const requested = latest(pr, 'requested', login); - const pushed = latest(pr, 'pushed'); - if (requested && requested > lastReview && pushed && pushed > lastReview) { + const pushedSince = lastReview.commit && pr.headRefOid && lastReview.commit !== pr.headRefOid; + if (requested && requested > lastReview.at && pushedSince) { return { kind: 're-review', start: requested, target: TARGET_HOURS.reReview }; } return null; diff --git a/ReviewSLA/owed.test.js b/ReviewSLA/owed.test.js index 6a77cd5..8abde59 100644 --- a/ReviewSLA/owed.test.js +++ b/ReviewSLA/owed.test.js @@ -124,9 +124,9 @@ test('when the PR below merges, the clock starts when the base changes', () => { test('the author pushing a fix and re-requesting review starts a 4-hour re-review clock', () => { const p = pr({ assignees: ['ghopper'], + headRefOid: 'bbb', timeline: [ - ['review', '2026-10-01T18:00:00Z', review('ghopper', 'CHANGES_REQUESTED')], - ['pushed', '2026-10-01T20:00:00Z'], + ['review', '2026-10-01T18:00:00Z', { ...review('ghopper', 'CHANGES_REQUESTED'), commit: 'aaa' }], ['requested', '2026-10-01T20:05:00Z', { login: 'ghopper' }], ], }); @@ -139,9 +139,9 @@ test('the author pushing a fix and re-requesting review starts a 4-hour re-revie test('a re-request with no push since the last review (a Graphite resubmit) starts nothing', () => { const p = pr({ assignees: ['ghopper'], + headRefOid: 'aaa', timeline: [ - ['pushed', '2026-10-01T17:30:00Z'], - ['review', '2026-10-01T18:00:00Z', review('ghopper', 'COMMENTED')], + ['review', '2026-10-01T18:00:00Z', { ...review('ghopper', 'COMMENTED'), commit: 'aaa' }], ['requested', '2026-10-01T20:05:00Z', { login: 'ghopper' }], ], }); From f6e777a477e82e97b8aa2d541854d086c57e3d76 Mon Sep 17 00:00:00 2001 From: Thomas Willson Date: Thu, 1 Oct 2026 17:12:49 -0700 Subject: [PATCH 3/5] Run the Review SLA job from a private repo, not this one This repo's run logs are public, and the job's would show who is out of office and activity in the private repos it reads. The scheduled workflow moves to a private repo; the README has it. Co-Authored-By: Claude Opus 5.5 --- .github/workflows/review-sla.yml | 59 -------------------------- ReviewSLA/README.md | 72 ++++++++++++++++++++++++++++++-- 2 files changed, 68 insertions(+), 63 deletions(-) delete mode 100644 .github/workflows/review-sla.yml diff --git a/.github/workflows/review-sla.yml b/.github/workflows/review-sla.yml deleted file mode 100644 index 2dfba28..0000000 --- a/.github/workflows/review-sla.yml +++ /dev/null @@ -1,59 +0,0 @@ -name: Review SLA - -on: - schedule: - # Every 30 minutes, 10:00 to 17:30 Pacific on weekdays, in daylight and standard time. The job - # skips runs outside business hours and on holidays. - - cron: '*/30 17-23 * * 1-5' - - cron: '*/30 0-1 * * 2-6' - # 10:00 Pacific, which posts the Slack digest: 17:00 UTC in daylight time, 18:00 in standard. - - cron: '0 17 * * 1-5' - - cron: '0 18 * * 1-5' - workflow_dispatch: - inputs: - digest: - description: Post the Slack digest on this run - type: boolean - default: false - -permissions: {} - -# Runs never overlap, so a reminder recorded by one is seen by the next. -concurrency: - group: review-sla - cancel-in-progress: false - -jobs: - sla: - # REVIEW_SLA_MODE (off, shadow, remind or enforce) switches it, and is the rollback. - if: ${{ vars.REVIEW_SLA_MODE != '' && vars.REVIEW_SLA_MODE != 'off' }} - runs-on: ubuntu-latest - steps: - - uses: actions/checkout@v4.1.2 - - uses: actions/create-github-app-token@v2 - id: app - with: - app-id: ${{ vars.STAFL_CI_APP_ID }} - private-key: ${{ secrets.STAFL_CI_PRIVATE_KEY }} - owner: StaflSystems - - uses: ./ReviewSLA - with: - token: ${{ steps.app.outputs.token }} - mode: ${{ vars.REVIEW_SLA_MODE }} - repos: >- - StaflLib - coit-tower-bms2000 - goldengate-bms2000-string - stafl-bms2000-template - anza-bms2000 - CustomGithubActions - pto-calendar-url: ${{ secrets.PTO_CALENDAR_URL }} - people: ${{ vars.REVIEW_PEOPLE }} - slack-token: ${{ secrets.REVIEW_SLA_SLACK_BOT_TOKEN }} - slack-channel: ${{ vars.REVIEW_SLA_SLACK_CHANNEL }} - confluence-url: ${{ vars.CONFLUENCE_URL }} - confluence-user: ${{ vars.CONFLUENCE_USER }} - confluence-token: ${{ secrets.CONFLUENCE_API_TOKEN }} - confluence-page-id: ${{ vars.REVIEW_SLA_PAGE_ID }} - dashboard-url: https://staflsystems.atlassian.net/wiki/spaces/EM/pages/${{ vars.REVIEW_SLA_PAGE_ID }} - digest: ${{ inputs.digest || 'false' }} diff --git a/ReviewSLA/README.md b/ReviewSLA/README.md index 96a54e3..2a07be8 100644 --- a/ReviewSLA/README.md +++ b/ReviewSLA/README.md @@ -5,8 +5,10 @@ who owes a review on every open PR and by when, and report mode, which measures ## The scheduled job -`.github/workflows/review-sla.yml` runs it every 30 minutes during business hours (10:00 to 17:00 -Pacific, business days), and on demand. Each run rebuilds every assignee's clock from the open +A scheduled workflow in a private repo runs it every 30 minutes during business hours (10:00 to +17:00 Pacific, business days), and on demand. It runs from a private repo, not this one, because +this repo's run logs are public: they'd show who is out of office, and activity in the private +repos the job reads. Each run rebuilds every assignee's clock from the open PRs' timelines; there's no other state. The repo variable `REVIEW_SLA_MODE` switches it, and is the rollback for every step: @@ -62,7 +64,7 @@ It fails safe: an API error is a warning, and that PR, repo, channel or page is | Name | Kind | What it is | | --- | --- | --- | -| `REVIEW_SLA_MODE` | Repo variable | `off`, `shadow`, `remind` or `enforce` | +| `REVIEW_SLA_MODE` | Repo variable, in the repo that runs the job | `off`, `shadow`, `remind` or `enforce` | | `STAFL_CI_APP_ID`, `STAFL_CI_PRIVATE_KEY` | Org variable, secret | The staflsystemsci app, installed on every repo checked, with Pull requests and Issues write and Members read | | `PTO_CALENDAR_URL` | Org secret | Rippling PTO calendar feed | | `REVIEW_PEOPLE` | Org variable | Names and Slack IDs (see `ReviewConfig/`) | @@ -70,7 +72,69 @@ It fails safe: an API error is a warning, and that PR, repo, channel or page is | `CONFLUENCE_URL`, `CONFLUENCE_USER`, `CONFLUENCE_API_TOKEN` | Org variables, secret | Base URL ending in `/wiki`, and the email and API token of an account that can edit the page. A scoped (service account) token uses `https://api.atlassian.com/ex/confluence//wiki` | | `REVIEW_SLA_PAGE_ID` | Org variable | The dashboard page | -The repos checked are listed in the workflow. +The org secrets and variables have to be visible to the repo that runs the job. The workflow, +which also lists the repos checked: + +```yaml +name: Review SLA + +on: + schedule: + # Every 30 minutes, 10:00 to 17:30 Pacific on weekdays, in daylight and standard time. The job + # skips runs outside business hours and on holidays. + - cron: '*/30 17-23 * * 1-5' + - cron: '*/30 0-1 * * 2-6' + # 10:00 Pacific, which posts the Slack digest: 17:00 UTC in daylight time, 18:00 in standard. + - cron: '0 17 * * 1-5' + - cron: '0 18 * * 1-5' + workflow_dispatch: + inputs: + digest: + description: Post the Slack digest on this run + type: boolean + default: false + +permissions: {} + +# Runs never overlap, so a reminder recorded by one is seen by the next. +concurrency: + group: review-sla + cancel-in-progress: false + +jobs: + sla: + # REVIEW_SLA_MODE (off, shadow, remind or enforce) switches it, and is the rollback. + if: ${{ vars.REVIEW_SLA_MODE != '' && vars.REVIEW_SLA_MODE != 'off' }} + runs-on: ubuntu-latest + steps: + - uses: actions/create-github-app-token@v2 + id: app + with: + app-id: ${{ vars.STAFL_CI_APP_ID }} + private-key: ${{ secrets.STAFL_CI_PRIVATE_KEY }} + owner: StaflSystems + - uses: StaflSystems/CustomGithubActions/ReviewSLA@main + with: + token: ${{ steps.app.outputs.token }} + mode: ${{ vars.REVIEW_SLA_MODE }} + repos: >- + StaflLib + coit-tower-bms2000 + goldengate-bms2000-string + stafl-bms2000-template + anza-bms2000 + CustomGithubActions + pto-calendar-url: ${{ secrets.PTO_CALENDAR_URL }} + people: ${{ vars.REVIEW_PEOPLE }} + slack-token: ${{ secrets.REVIEW_SLA_SLACK_BOT_TOKEN }} + slack-channel: ${{ vars.REVIEW_SLA_SLACK_CHANNEL }} + confluence-url: ${{ vars.CONFLUENCE_URL }} + confluence-user: ${{ vars.CONFLUENCE_USER }} + confluence-token: ${{ secrets.CONFLUENCE_API_TOKEN }} + confluence-page-id: ${{ vars.REVIEW_SLA_PAGE_ID }} + dashboard-url: https://staflsystems.atlassian.net/wiki/spaces/EM/pages/${{ vars.REVIEW_SLA_PAGE_ID }} + digest: ${{ inputs.digest || 'false' }} +``` ## Report mode From 5327605cd8f5a0e939959efb35ddb2ec23ece99a Mon Sep 17 00:00:00 2001 From: Thomas Willson Date: Thu, 1 Oct 2026 17:16:37 -0700 Subject: [PATCH 4/5] ReviewSLA: read reviewed commits from REST, which needs no Contents permission GraphQL's PullRequestReview.commit is refused in private repos without the app's Contents permission. REST's commit_id isn't, so it's read for the PRs where a re-review could be running: a re-request after that person's review. Co-Authored-By: Claude Opus 5.5 --- ReviewSLA/github.js | 35 +++++++++++++++++++++++++++++++---- ReviewSLA/owed.js | 4 ++-- ReviewSLA/run.js | 2 +- ReviewSLA/run.test.js | 24 ++++++++++++++++++++++-- 4 files changed, 56 insertions(+), 9 deletions(-) diff --git a/ReviewSLA/github.js b/ReviewSLA/github.js index 6131d02..0587ce7 100644 --- a/ReviewSLA/github.js +++ b/ReviewSLA/github.js @@ -18,7 +18,7 @@ const TIMELINE_FIELDS = ` __typename ... on ReadyForReviewEvent { createdAt } ... on ConvertToDraftEvent { createdAt } - ... on PullRequestReview { state submittedAt body comments { totalCount } author { __typename login } commit { oid } } + ... on PullRequestReview { databaseId state submittedAt body comments { totalCount } author { __typename login } } ... on AssignedEvent { createdAt assignee { ... on User { login } } } ... on UnassignedEvent { createdAt assignee { ... on User { login } } } ... on ReviewRequestedEvent { createdAt requestedReviewer { ... on User { login } } } @@ -66,7 +66,8 @@ function toEvent(item) { state: item.state, body: item.body, comments: item.comments.totalCount, - commit: item.commit?.oid ?? null, + id: item.databaseId, + commit: null, }; case 'AssignedEvent': case 'UnassignedEvent': @@ -101,8 +102,32 @@ function toOpenPr(repo, node, items) { }; } +// Whether a re-review clock could be running: a re-request of someone after their own review. +function mayOweReReview(pr) { + if (pr.isDraft) return false; + const reviewed = new Map(); + for (const e of [...pr.timeline].sort((a, b) => a.at - b.at)) { + if (e.type === 'review' && e.author) reviewed.set(e.author.login, e.at); + if (e.type === 'requested' && reviewed.has(e.login)) return true; + } + return false; +} + +// The commit each review was left on, which tells whether the author has pushed since. GraphQL +// only gives it with the app's Contents permission in private repos; REST's commit_id doesn't +// need it. On failure the commits stay unknown, so no re-review clock starts. +async function addReviewCommits({ github, core, owner, pr }) { + try { + const reviews = await github.paginate(github.rest.pulls.listReviews, { owner, repo: pr.repo, pull_number: pr.number, per_page: 100 }); + const commits = new Map(reviews.map((r) => [r.id, r.commit_id])); + for (const e of pr.timeline) if (e.type === 'review') e.commit = commits.get(e.id) ?? null; + } catch (error) { + core.warning(`${pr.repo}#${pr.number}: couldn't read which commits were reviewed (${error.message}); no re-review clocks on it this run.`); + } +} + // Earlier timeline pages are read for PRs with more than 100 events, up to `maxPages` in all. -async function fetchOpenPrs({ github, owner, repo, maxPages = 5 }) { +async function fetchOpenPrs({ github, core, owner, repo, maxPages = 5 }) { const prs = []; for (let after = null; ; ) { const data = await github.graphql(OPEN_PRS, { owner, name: repo, after }); @@ -115,7 +140,9 @@ async function fetchOpenPrs({ github, owner, repo, maxPages = 5 }) { items = [...earlier.repository.pullRequest.timelineItems.nodes, ...items]; info = earlier.repository.pullRequest.timelineItems.pageInfo; } - prs.push(toOpenPr(repo, node, items)); + const pr = toOpenPr(repo, node, items); + if (mayOweReReview(pr)) await addReviewCommits({ github, core, owner, pr }); + prs.push(pr); } if (!page.pageInfo.hasNextPage) break; after = page.pageInfo.endCursor; diff --git a/ReviewSLA/owed.js b/ReviewSLA/owed.js index abedc6a..b9be9c0 100644 --- a/ReviewSLA/owed.js +++ b/ReviewSLA/owed.js @@ -12,8 +12,8 @@ // assignment, their approval of the PR below, and the PR's base changing (the PR below merging). // Requesting changes doesn't move them up the stack. Once they've responded, a re-request from the // author starts a re-review clock, but only if the PR's head has moved since their last review, so -// Graphite re-requesting on every `gt submit` doesn't. (Comparing commits rather than push times -// also means the app needs no Contents permission, and a rebase can't fake a date.) +// Graphite re-requesting on every `gt submit` doesn't. Commits are compared rather than push +// times, which need the app's Contents permission and which a rebase rewrites. const { addBusinessHours, businessHoursBetween } = require('../ReviewConfig/hours.js'); const { byTime, countsAsResponse, hasSla, isBot, openedAsDraft, targetHours, TARGET_HOURS } = require('./clock.js'); diff --git a/ReviewSLA/run.js b/ReviewSLA/run.js index 55cd72a..7414f5a 100644 --- a/ReviewSLA/run.js +++ b/ReviewSLA/run.js @@ -127,7 +127,7 @@ module.exports = async function run({ github, core, inputs, now = new Date(), fe let clocks = []; for (const repo of repos) { try { - const open = await fetchOpenPrs({ github, owner, repo }); + const open = await fetchOpenPrs({ github, core, owner, repo }); prs.push(...open); clocks.push(...owedClocks(open, { now, holidays })); } catch (error) { diff --git a/ReviewSLA/run.test.js b/ReviewSLA/run.test.js index 5b9b038..3a50991 100644 --- a/ReviewSLA/run.test.js +++ b/ReviewSLA/run.test.js @@ -14,13 +14,14 @@ const PTO_URL = 'https://calendar.example/pto.ics'; const OPENED = '2026-10-01T17:00:00Z'; const userNode = (login) => ({ __typename: 'User', login }); -const reviewItem = (login, state, iso) => ({ __typename: 'PullRequestReview', state, submittedAt: iso, body: 'ok', comments: { totalCount: 0 }, author: userNode(login) }); +const reviewItem = (login, state, iso, databaseId = 1) => ({ __typename: 'PullRequestReview', databaseId, state, submittedAt: iso, body: 'ok', comments: { totalCount: 0 }, author: userNode(login) }); +const requestedItem = (login, iso) => ({ __typename: 'ReviewRequestedEvent', createdAt: iso, requestedReviewer: userNode(login) }); // A fake org: open PRs (as GraphQL nodes, rebuilt on every query so comments written by one run // are read by the next), teams, Slack, Confluence and the PTO calendar. function world({ prs, out = [], failRepo, failComments = false }) { const calls = { dms: [], posts: [], comments: [], edits: [], assign: [], unassign: [], requested: [], puts: [], graphql: 0 }; - const state = new Map(prs.map((p) => [p.number, { repo: 'StaflLib', author: 'alovelace', base: 'main', isDraft: false, additions: 300, timeline: [], assignees: [], comments: [], ...p }])); + const state = new Map(prs.map((p) => [p.number, { repo: 'StaflLib', author: 'alovelace', base: 'main', isDraft: false, additions: 300, timeline: [], assignees: [], comments: [], headRefOid: 'head', reviewCommits: {}, ...p }])); let commentId = 100; let page = { title: 'Review Dashboard', version: { number: 3, message: '' } }; const asNode = (p) => ({ @@ -30,6 +31,7 @@ function world({ prs, out = [], failRepo, failComments = false }) { isDraft: p.isDraft, additions: p.additions, headRefName: p.head, + headRefOid: p.headRefOid, baseRefName: p.base, createdAt: OPENED, author: userNode(p.author), @@ -74,6 +76,10 @@ function world({ prs, out = [], failRepo, failComments = false }) { }, pulls: { requestReviewers: async ({ pull_number, reviewers }) => calls.requested.push([pull_number, ...reviewers]), + listReviews: async ({ pull_number }) => { + calls.listReviews = (calls.listReviews ?? 0) + 1; + return { data: Object.entries(state.get(pull_number).reviewCommits).map(([id, commit_id]) => ({ id: Number(id), commit_id })) }; + }, }, }, }; @@ -170,6 +176,20 @@ test('remind DMs each overdue assignee once, however many runs see it', async () assert.strictEqual(w.calls.comments.length, 1); }); +test('a re-request after a push since the review starts a re-review clock; without one, nothing', async () => { + const timeline = [reviewItem('ghopper', 'CHANGES_REQUESTED', '2026-10-01T18:00:00Z', 7), reviewItem('kjohnson', 'APPROVED', '2026-10-01T18:00:00Z', 8), requestedItem('ghopper', '2026-10-01T20:00:00Z')]; + const pushed = world({ prs: [onePr({ timeline, headRefOid: 'bbb', reviewCommits: { 7: 'aaa', 8: 'aaa' } })] }); + await go(pushed, { now: '2026-10-01T21:00:00Z' }); + assert.match(pushed.calls.comments[0].body, /\| `ghopper` \| Re-review \| Thu Oct 1, 17:00 PT \| {2}\|/); + const resubmitted = world({ prs: [onePr({ timeline, headRefOid: 'aaa', reviewCommits: { 7: 'aaa', 8: 'aaa' } })] }); + await go(resubmitted, { now: '2026-10-01T21:00:00Z' }); + assert.deepStrictEqual(resubmitted.calls.comments, []); + // Only PRs with a re-request after a review need the extra call. + const plain = world({ prs: [onePr()] }); + await go(plain, { now: '2026-10-01T21:00:00Z' }); + assert.strictEqual(plain.calls.listReviews, undefined); +}); + test('no comment is created for a PR nobody owes a review on', async () => { const w = world({ prs: [onePr({ timeline: [reviewItem('ghopper', 'APPROVED', '2026-10-01T18:00:00Z'), reviewItem('kjohnson', 'APPROVED', '2026-10-01T18:00:00Z')] })] }); await go(w, { now: '2026-10-02T00:00:00Z' }); From ab408ac2a055eea803d48c2142334c3800382802 Mon Sep 17 00:00:00 2001 From: Thomas Willson Date: Thu, 1 Oct 2026 17:37:34 -0700 Subject: [PATCH 5/5] ReviewSLA: keep approved PRs on reassignment; don't name PTO on public PRs An assignee who is out was replaced on every PR in the stack, including ones they'd already approved, so their replacement owed reviews that were done. Now only the PRs they haven't approved change hands. The reassignment notice said the person was out of office, which on a public repo's PR anyone can read. There it now says only that they aren't available. Co-Authored-By: Claude Opus 5.5 --- ReviewSLA/README.md | 6 ++++-- ReviewSLA/github.js | 6 ++++-- ReviewSLA/run.js | 14 +++++++++----- ReviewSLA/run.test.js | 28 ++++++++++++++++++++++++++-- 4 files changed, 43 insertions(+), 11 deletions(-) diff --git a/ReviewSLA/README.md b/ReviewSLA/README.md index 2a07be8..2d9507e 100644 --- a/ReviewSLA/README.md +++ b/ReviewSLA/README.md @@ -47,10 +47,12 @@ Bot PRs and Graphite merge-queue PRs have no clocks. - **Reminder DMs** (`remind`): one per clock, when it's due, to the assignee's Slack ID in `REVIEW_PEOPLE`. - **Out-of-office reassignment** (`remind`): an assignee the PTO calendar has out on any day from - today to the day their review is due is replaced on every PR in the stack they're assigned to. + today to the day their review is due is replaced on every PR in the stack they're assigned to + and haven't approved. Their approvals stand. A domain approver is replaced from the domain team and a rotation reviewer from the rotation team, by the same least-loaded pick AssignReviewers uses (`ReviewConfig/pick.js`). The other assignee is - kept, and a comment on the lowest of those PRs says who took over and why. + kept, and a comment on the lowest of those PRs says who took over and why. In a public repo it + says only that they aren't available, since anyone can read it. - **The digest:** posted to the `REVIEW_SLA_SLACK_CHANNEL` channel by the 10:00 Pacific run (or a manual run with **digest** ticked): overdue reviews by person, and how many more are due today. diff --git a/ReviewSLA/github.js b/ReviewSLA/github.js index 0587ce7..33455fc 100644 --- a/ReviewSLA/github.js +++ b/ReviewSLA/github.js @@ -27,6 +27,7 @@ const TIMELINE_FIELDS = ` const OPEN_PRS = `query($owner: String!, $name: String!, $after: String) { repository(owner: $owner, name: $name) { + isPrivate pullRequests(states: OPEN, first: 30, after: $after) { pageInfo { hasNextPage endCursor } nodes { @@ -81,10 +82,11 @@ function toEvent(item) { } } -function toOpenPr(repo, node, items) { +function toOpenPr(repo, node, items, isPrivate = true) { const sticky = node.comments.nodes.find((c) => c.body?.startsWith(MARKER)); return { repo, + isPrivate, number: node.number, url: node.url, title: node.title, @@ -140,7 +142,7 @@ async function fetchOpenPrs({ github, core, owner, repo, maxPages = 5 }) { items = [...earlier.repository.pullRequest.timelineItems.nodes, ...items]; info = earlier.repository.pullRequest.timelineItems.pageInfo; } - const pr = toOpenPr(repo, node, items); + const pr = toOpenPr(repo, node, items, data.repository.isPrivate !== false); if (mayOweReReview(pr)) await addReviewCommits({ github, core, owner, pr }); prs.push(pr); } diff --git a/ReviewSLA/run.js b/ReviewSLA/run.js index 7414f5a..51e693d 100644 --- a/ReviewSLA/run.js +++ b/ReviewSLA/run.js @@ -18,7 +18,7 @@ const { hasSla } = require('./clock.js'); const { parseState, reminderKey, renderComment } = require('./comment.js'); const { renderDashboard, updateDashboard } = require('./confluence.js'); const { fetchOpenPrs } = require('./github.js'); -const { assigneesOf, owedClocks, readySince, stackOf } = require('./owed.js'); +const { approvedAt, assigneesOf, owedClocks, readySince, stackOf } = require('./owed.js'); const { digestText, postMessage, reminderText } = require('./slack.js'); const MODES = ['off', 'shadow', 'remind', 'enforce']; @@ -69,8 +69,9 @@ function outBefore(out, login, due, now) { return false; } -// Replaces `login` with someone from the same team on every PR in the stack they're assigned to. -// The domain approver is replaced from the domain team, the rotation reviewer from the rotation team. +// Replaces `login` with someone from the same team on every PR in the stack they're assigned to +// and haven't approved; their approvals stand. The domain approver is replaced from the domain +// team, the rotation reviewer from the rotation team. async function reassign({ github, core, owner, inputs, teams, clock, stack, out, now }) { const { pr, login } = clock; const others = assigneesOf(pr).filter((l) => l !== login); @@ -86,7 +87,8 @@ async function reassign({ github, core, owner, inputs, teams, clock, stack, out, core.warning(`${pr.repo}#${pr.number}: ${login} is out before their review is due, but no one else in ${team} is available.`); return []; } - const changed = stack.filter((p) => assigneesOf(p).includes(login)); + const changed = stack.filter((p) => assigneesOf(p).includes(login) && !approvedAt(p, login)); + if (changed.length === 0) return []; for (const p of changed) { await github.rest.issues.removeAssignees({ owner, repo: p.repo, issue_number: p.number, assignees: [login] }); await github.rest.issues.addAssignees({ owner, repo: p.repo, issue_number: p.number, assignees: [chosen.login] }); @@ -98,12 +100,14 @@ async function reassign({ github, core, owner, inputs, teams, clock, stack, out, } } const numbers = changed.map((p) => `#${p.number}`).join(', '); + // A public repo's PRs are visible to anyone, so they don't say why. + const reason = pr.isPrivate === false ? `\`${login}\` isn't available` : `the PTO calendar has \`${login}\` out`; await github.rest.issues.createComment({ owner, repo: pr.repo, issue_number: changed[0].number, body: - `**Review SLA:** the PTO calendar has \`${login}\` out before their review is due, so @${chosen.login} ` + + `**Review SLA:** ${reason} before their review is due, so @${chosen.login} ` + `takes over from them as assignee on ${numbers}, picked from ${team} by fewest open assigned PRs.`, }); core.info(`${pr.repo}: reassigned ${login} to ${chosen.login} on ${numbers} (out of office).`); diff --git a/ReviewSLA/run.test.js b/ReviewSLA/run.test.js index 3a50991..dcadcfa 100644 --- a/ReviewSLA/run.test.js +++ b/ReviewSLA/run.test.js @@ -19,7 +19,7 @@ const requestedItem = (login, iso) => ({ __typename: 'ReviewRequestedEvent', cre // A fake org: open PRs (as GraphQL nodes, rebuilt on every query so comments written by one run // are read by the next), teams, Slack, Confluence and the PTO calendar. -function world({ prs, out = [], failRepo, failComments = false }) { +function world({ prs, out = [], failRepo, failComments = false, isPrivate = true }) { const calls = { dms: [], posts: [], comments: [], edits: [], assign: [], unassign: [], requested: [], puts: [], graphql: 0 }; const state = new Map(prs.map((p) => [p.number, { repo: 'StaflLib', author: 'alovelace', base: 'main', isDraft: false, additions: 300, timeline: [], assignees: [], comments: [], headRefOid: 'head', reviewCommits: {}, ...p }])); let commentId = 100; @@ -48,7 +48,7 @@ function world({ prs, out = [], failRepo, failComments = false }) { } if (vars.name === failRepo) throw new Error('HTTP 502'); const nodes = [...state.values()].filter((p) => p.repo === vars.name).map(asNode); - return { repository: { pullRequests: { pageInfo: { hasNextPage: false }, nodes } } }; + return { repository: { isPrivate, pullRequests: { pageInfo: { hasNextPage: false }, nodes } } }; }, rest: { teams: { @@ -211,6 +211,30 @@ test('an assignee out before their review is due is replaced across the stack, a assert.match(notice.body, /`ghopper` out before their review is due, so @mhamilton takes over from them as assignee on #1, #2/); }); +test('an assignee who is out keeps the PRs they already approved', async () => { + // ghopper approved bottom PR #1 and owes a review on #2. + const w = world({ + prs: [ + onePr({ timeline: [reviewItem('ghopper', 'APPROVED', '2026-10-01T17:30:00Z')] }), + { number: 2, head: 'feature/b', base: 'feature/a', assignees: ['ghopper', 'kjohnson'] }, + ], + out: [['Person ghopper', '2026-10-01']], + }); + await go(w, { now: '2026-10-01T18:00:00Z' }); + assert.deepStrictEqual(w.calls.unassign, [[2, 'ghopper']]); + assert.deepStrictEqual(w.pr(1).assignees.sort(), ['ghopper', 'kjohnson']); + const notice = w.calls.comments.find((c) => !c.body.startsWith(MARKER)); + assert.strictEqual(notice.issue_number, 2); +}); + +test('on a public repo, the reassignment notice doesn\'t say the person is out', async () => { + const w = world({ prs: [onePr()], out: [['Person ghopper', '2026-10-01']], isPrivate: false }); + await go(w, { now: '2026-10-01T18:00:00Z' }); + const notice = w.calls.comments.find((c) => !c.body.startsWith(MARKER)); + assert.match(notice.body, /^\*\*Review SLA:\*\* `ghopper` isn't available before their review is due, so @mhamilton takes over/); + assert.doesNotMatch(notice.body, /PTO|out/); +}); + test('someone out after their review is due is left alone', async () => { const w = world({ prs: [onePr()], out: [['Person ghopper', '2026-10-02']] }); await go(w, { now: '2026-10-01T18:00:00Z' });