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..2d9507e 100644 --- a/ReviewSLA/README.md +++ b/ReviewSLA/README.md @@ -1,8 +1,142 @@ # 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 + +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: + +| 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 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. + +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 + 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. 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. +- **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, 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/`) | +| `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 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 @@ -55,7 +189,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..33455fc --- /dev/null +++ b/ReviewSLA/github.js @@ -0,0 +1,155 @@ +// 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', + 'BASE_REF_CHANGED_EVENT', +].join(', '); + +const TIMELINE_FIELDS = ` + pageInfo { hasPreviousPage startCursor } + nodes { + __typename + ... on ReadyForReviewEvent { createdAt } + ... on ConvertToDraftEvent { createdAt } + ... 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 } } } + ... on BaseRefChangedEvent { createdAt } + }`; + +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 { + number url title isDraft additions headRefName headRefOid 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, + id: item.databaseId, + commit: 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 'BaseRefChangedEvent': + return { type: 'base', at: at(item.createdAt) }; + default: + return null; + } +} + +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, + isDraft: node.isDraft, + additions: node.additions, + headRefName: node.headRefName, + headRefOid: node.headRefOid, + 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, + }; +} + +// 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, core, 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; + } + const pr = toOpenPr(repo, node, items, data.repository.isPrivate !== false); + if (mayOweReReview(pr)) await addReviewCommits({ github, core, owner, pr }); + prs.push(pr); + } + 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..b9be9c0 --- /dev/null +++ b/ReviewSLA/owed.js @@ -0,0 +1,119 @@ +// 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, 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: '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 PR's head has moved since their last review, so +// 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'); + +// 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); + const requested = latest(pr, 'requested', login); + 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; +} + +// 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..8abde59 --- /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'], + headRefOid: 'bbb', + timeline: [ + ['review', '2026-10-01T18:00:00Z', { ...review('ghopper', 'CHANGES_REQUESTED'), commit: 'aaa' }], + ['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'], + headRefOid: 'aaa', + timeline: [ + ['review', '2026-10-01T18:00:00Z', { ...review('ghopper', 'COMMENTED'), commit: 'aaa' }], + ['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..51e693d --- /dev/null +++ b/ReviewSLA/run.js @@ -0,0 +1,239 @@ +// 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 { approvedAt, 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 +// 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); + 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) && !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] }); + 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(', '); + // 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:** ${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).`); + 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, core, 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..dcadcfa --- /dev/null +++ b/ReviewSLA/run.test.js @@ -0,0 +1,295 @@ +// 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, 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, 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; + 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, + headRefOid: p.headRefOid, + 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: { isPrivate, 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]), + 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 })) }; + }, + }, + }, + }; + 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('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' }); + 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('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' }); + 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 };