Add ReviewSLA step 1: first-response clocks and report mode

.github/workflows/assign-reviewers-tests.yml → .github/workflows/review-tests.yml

@@ -1,11 +1,11 @@
-name: AssignReviewers tests
+name: Review action tests
 
 on:
   pull_request:
-    paths: [AssignReviewers/**, ReviewConfig/**]
+    paths: [AssignReviewers/**, ReviewConfig/**, ReviewSLA/**]
   push:
     branches: [main]
-    paths: [AssignReviewers/**, ReviewConfig/**]
+    paths: [AssignReviewers/**, ReviewConfig/**, ReviewSLA/**]
 
 permissions:
   contents: read
@@ -18,4 +18,4 @@ jobs:
       - uses: actions/setup-node@v4
         with:
           node-version: 20
-      - run: node --test AssignReviewers/ ReviewConfig/
+      - run: node --test AssignReviewers/ ReviewConfig/ ReviewSLA/

.gitignore

@@ -1,2 +1,3 @@
 .DS_Store
 ci-metrics-data/
+review-sla-report/

ReviewConfig/README.md

@@ -1,7 +1,7 @@
 # Review config
 
-Shared by the review actions (`AssignReviewers`, and the review SLA job to come): who people are,
-when they're out, and which days are company holidays.
+Shared by the review actions (`AssignReviewers` and `ReviewSLA`): who people are, when they're out,
+which days are company holidays, and what counts as business hours.
 
 ## People
 
@@ -26,6 +26,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 | — |
 
 ## Who is out
 
@@ -49,6 +50,10 @@ carry on as if nobody is out.
 Business days are Monday to Friday in Pacific time, minus the holidays in `holidays.json`. A year
 with no list gets a warning on every run, and every weekday counts until it's added.
 
+Business hours, which the review SLA is measured in, are 10:00 to 17:00 Pacific on business days.
+Start and end times vary across the team, so the band is narrower than anyone's working day, and a
+business day is 7 hours.
+
 ## Tests
 
 ```bash

ReviewConfig/hours.js

@@ -0,0 +1,63 @@
+// Business hours for the review SLA: 10:00 to 17:00 Pacific on business days (Monday to Friday,
+// minus the holidays in holidays.json). Start and end times vary across the team, so the band is
+// narrower than anyone's working day, and a business day is 7 hours.
+
+const { TIME_ZONE, addDays, holidaysFor, isWeekend, pacificDate } = require('./pto.js');
+
+const OPENS = 10;
+const CLOSES = 17;
+const HOUR_MS = 3600 * 1000;
+
+function isBusinessDay(date, holidays) {
+  return !isWeekend(date) && !holidaysFor(holidays, date.slice(0, 4))?.has(date);
+}
+
+// Pacific's UTC offset on a date, in hours (-7 or -8). DST changes at 02:00, so it's the same
+// all through business hours.
+function pacificOffset(date) {
+  const name = new Intl.DateTimeFormat('en-US', { timeZone: TIME_ZONE, timeZoneName: 'longOffset' })
+    .formatToParts(new Date(`${date}T12:00:00Z`))
+    .find((part) => part.type === 'timeZoneName').value;
+  const match = /GMT([+-])(\d{2}):(\d{2})/.exec(name);
+  return match ? (match[1] === '-' ? -1 : 1) * (Number(match[2]) + Number(match[3]) / 60) : 0;
+}
+
+// The instant it's `hour`:00 Pacific on `date`.
+function pacificTime(date, hour) {
+  return new Date(Date.parse(`${date}T00:00:00Z`) + (hour - pacificOffset(date)) * HOUR_MS);
+}
+
+// [opens, closes] on a business day, or null on a weekend or holiday.
+function businessWindow(date, holidays) {
+  return isBusinessDay(date, holidays) ? [pacificTime(date, OPENS), pacificTime(date, CLOSES)] : null;
+}
+
+// Business hours between two instants. 0 if `end` isn't after `start`.
+function businessHoursBetween(start, end, holidays) {
+  if (end <= start) return 0;
+  let ms = 0;
+  const last = pacificDate(end);
+  for (let date = pacificDate(start); date <= last; date = addDays(date, 1)) {
+    const window = businessWindow(date, holidays);
+    if (!window) continue;
+    const from = Math.max(start, window[0]);
+    const to = Math.min(end, window[1]);
+    if (to > from) ms += to - from;
+  }
+  return ms / HOUR_MS;
+}
+
+// The instant `hours` business hours after `start`: when a review started then is due.
+function addBusinessHours(start, hours, holidays) {
+  let remaining = hours * HOUR_MS;
+  for (let date = pacificDate(start); ; date = addDays(date, 1)) {
+    const window = businessWindow(date, holidays);
+    if (!window) continue;
+    const from = Math.max(start, window[0]);
+    if (from >= window[1]) continue;
+    if (from + remaining <= window[1]) return new Date(from + remaining);
+    remaining -= window[1] - from;
+  }
+}
+
+module.exports = { addBusinessHours, businessHoursBetween, isBusinessDay, pacificTime };

ReviewConfig/hours.test.js

@@ -0,0 +1,46 @@
+// Run with: node --test ReviewConfig/
+const test = require('node:test');
+const assert = require('node:assert');
+const { addBusinessHours, businessHoursBetween } = require('./hours.js');
+
+const HOLIDAYS = { 2026: [{ date: '2026-11-26' }, { date: '2026-11-27' }] };
+const at = (iso) => new Date(iso);
+
+test('a small PR marked ready at 16:30 Friday is due at 13:30 Monday', () => {
+  // 2026-10-02 is a Friday; Pacific is UTC-7 in October.
+  assert.deepStrictEqual(addBusinessHours(at('2026-10-02T23:30:00Z'), 4, HOLIDAYS), at('2026-10-05T20:30:00Z'));
+});
+
+test('a standard PR is due one business day later', () => {
+  // 11:00 Thursday + 7 h = 11:00 Friday.
+  assert.deepStrictEqual(addBusinessHours(at('2026-10-01T18:00:00Z'), 7, HOLIDAYS), at('2026-10-02T18:00:00Z'));
+});
+
+test('holidays and weekends don\'t count', () => {
+  // 15:00 Wednesday before Thanksgiving + 4 h: 2 h Wednesday, then Monday 10:00 + 2 h. PST from Nov 1.
+  assert.deepStrictEqual(addBusinessHours(at('2026-11-25T23:00:00Z'), 4, HOLIDAYS), at('2026-11-30T20:00:00Z'));
+  assert.strictEqual(businessHoursBetween(at('2026-11-25T23:00:00Z'), at('2026-11-30T20:00:00Z'), HOLIDAYS), 4);
+});
+
+test('time before opening and after closing doesn\'t count', () => {
+  // 08:00 to 18:00 Pacific on a Thursday.
+  assert.strictEqual(businessHoursBetween(at('2026-10-01T15:00:00Z'), at('2026-10-02T01:00:00Z'), HOLIDAYS), 7);
+  // Ready at 07:00, due at 14:00 for a small PR.
+  assert.deepStrictEqual(addBusinessHours(at('2026-10-01T14:00:00Z'), 4, HOLIDAYS), at('2026-10-01T21:00:00Z'));
+});
+
+test('a review after closing on the day it was due is no later than one first thing next morning', () => {
+  const start = at('2026-10-01T23:00:00Z'); // 16:00 Thursday
+  assert.strictEqual(businessHoursBetween(start, at('2026-10-02T04:00:00Z'), HOLIDAYS), 1); // 21:00
+  assert.strictEqual(businessHoursBetween(start, at('2026-10-02T17:00:00Z'), HOLIDAYS), 1); // 10:00 Friday
+});
+
+test('the switch to standard time doesn\'t shift the window', () => {
+  // Nov 1, 2026 is the Sunday clocks go back. Friday Oct 30 10:00 PDT to Monday Nov 2 10:00 PST.
+  assert.strictEqual(businessHoursBetween(at('2026-10-30T17:00:00Z'), at('2026-11-02T18:00:00Z'), HOLIDAYS), 7);
+});
+
+test('no time, or time running backwards, is 0', () => {
+  assert.strictEqual(businessHoursBetween(at('2026-10-01T20:00:00Z'), at('2026-10-01T20:00:00Z'), HOLIDAYS), 0);
+  assert.strictEqual(businessHoursBetween(at('2026-10-01T20:00:00Z'), at('2026-10-01T19:00:00Z'), HOLIDAYS), 0);
+});

ReviewConfig/pto.js

@@ -12,12 +12,11 @@ const path = require('node:path');
 const TIME_ZONE = 'America/Los_Angeles';
 const NOT_OUT = new Set(['work from home']);
 
-function readJson(file) {
+// The checked-in company paid holidays: { "<year>": [{ "date": "YYYY-MM-DD", "name": "..." }] }.
+function readHolidays(file = path.join(__dirname, 'holidays.json')) {
   return JSON.parse(fs.readFileSync(file, 'utf8'));
 }
 
-const defaultHolidaysFile = path.join(__dirname, 'holidays.json');
-
 // 'YYYY-MM-DD' of an instant, in Pacific time.
 function pacificDate(instant) {
   return new Intl.DateTimeFormat('en-CA', { timeZone: TIME_ZONE }).format(instant);
@@ -141,7 +140,7 @@ async function awaySoon({
   people,
   now = new Date(),
   fetch = globalThis.fetch,
-  holidays = readJson(defaultHolidaysFile),
+  holidays = readHolidays(),
 }) {
   if (!url) {
     core.info('No PTO calendar configured; not checking who is out.');
@@ -182,4 +181,15 @@ async function awaySoon({
   return away;
 }
 
-module.exports = { awaySoon, outDates, nextBusinessDay, pacificDate, parseIcs };
+module.exports = {
+  TIME_ZONE,
+  addDays,
+  awaySoon,
+  holidaysFor,
+  isWeekend,
+  nextBusinessDay,
+  outDates,
+  pacificDate,
+  parseIcs,
+  readHolidays,
+};

ReviewSLA/README.md

@@ -0,0 +1,65 @@
+# 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.
+
+## Report mode
+
+Weekly first-response numbers for a date range. Like CIMetrics, it isn't an action; run it locally
+with an authenticated `gh`:
+
+```bash
+node ReviewSLA/report.js StaflLib coit-tower-bms2000 --since 2026-06-26 --until 2026-09-24
+```
+
+It writes `review_sla_<since>_<until>.md` and a `prs.tsv` with one row per PR to `--out` (default
+`review-sla-report/`). Dates are Pacific and both ends are inclusive. Each PR counts in the week
+(Monday to Sunday) its clock started, and events after `--until` are ignored, so a past range reads
+the same however long after it the report is run.
+
+Each row has two sets of columns:
+
+| Columns | Clock starts | Clock stops | Measured in | Hit |
+| --- | --- | --- | --- | --- |
+| RFC | First ready event, or creation for a PR opened ready | First review by anyone but the author | Wall-clock hours | Within 8 h |
+| SLA | Each time the PR is marked ready (or opened ready); going back to draft stops it | First review that counts | Business hours | Within target |
+
+The RFC columns measure the way the RFC's baseline did, so its numbers can be checked; PRs whose
+first review came before that start aren't timed. The SLA columns follow the SLA's clock rules:
+
+- **Business hours** are 10:00 to 17:00 Pacific on business days (see `ReviewConfig/`), so a
+  business day is 7 hours.
+- **Targets** are 4 business hours for a PR under 250 added lines and 7 (one business day)
+  otherwise.
+- **A review counts** if it's an approval, changes requested, or a comment review with a body or at
+  least one inline comment. Issue comments, reactions, the author's own reviews and bots' reviews
+  don't.
+- **Within target** also counts PRs still unreviewed past their target, or closed unreviewed after
+  it, as misses. One unreviewed and still under its target isn't counted either way.
+- PRs reviewed while still drafts aren't timed, and bot PRs and Graphite merge-queue PRs are left
+  out.
+
+PRs created up to 60 days before `--since` are fetched, in case they were marked ready in range.
+Only each PR's first 100 ready, draft and review events are read; a PR with more and no review in
+the first 100 is flagged on stderr.
+
+### Checking it against the RFC
+
+Over Jun 26 to Sep 24, 2026, the RFC columns reproduce the RFC exactly for the PRs it measured:
+StaflLib's 139 come out at a 64.5 h median, 232.3 h p90 and 21% within 8 h, and coit-tower's 71 at
+5.4 h, 29.5 h and 70%. The full StaflLib run times 173, because the RFC sampled the 300 most recent
+PRs by creation date, and 34 PRs opened before Jun 26 were marked ready during the range.
+
+## Files
+
+| File | What it does |
+| --- | --- |
+| `clock.js` | Rebuilds a PR's first-response clock from its ready and draft events and its reviews |
+| `report.js` | Report mode: fetches PRs and writes the weekly report |
+
+## Tests
+
+```bash
+node --test ReviewSLA/ ReviewConfig/
+```

ReviewSLA/clock.js

@@ -0,0 +1,94 @@
+// A PR's first-response clock (review RFC, Proposal 2), rebuilt from its timeline.
+//
+// A PR here is { number, author, createdAt, closedAt, isDraft, additions, headRefName, timeline },
+// where `timeline` holds the PR's ready and draft events and its reviews, in any order:
+//   { type: 'ready' | 'draft', at }
+//   { type: 'review', at, author: { login, bot }, state, body, comments }
+// `author` is { login, bot }. Times are Dates.
+//
+// Two measures are kept side by side:
+// - `rfc` is how the RFC measured its baseline: from the first ready event (or creation, for a PR
+//   never marked ready) to the first review by anyone but the author, in wall-clock hours. PRs
+//   whose first review came before that start aren't timed.
+// - `sla` follows the SLA's clock rules: the clock runs while the PR is ready and restarts each
+//   time it's marked ready again, and only a review that counts stops it. It's measured in business
+//   hours against the PR's target.
+
+const { businessHoursBetween } = require('../ReviewConfig/hours.js');
+
+const SMALL_PR_LINES = 250;
+const TARGET_HOURS = { small: 4, standard: 7 };
+const HOUR_MS = 3600 * 1000;
+
+function isBot(actor) {
+  return !actor || actor.bot || actor.login.endsWith('[bot]');
+}
+
+// Bots, Graphite's merge-queue PRs and their speculative copies have no SLA.
+function hasSla(pr) {
+  return !isBot(pr.author) && pr.author.login !== 'graphite-app' && !pr.headRefName.startsWith('gtmq_');
+}
+
+// An approval, changes requested, or a comment review with a body or at least one inline comment.
+// A dismissed review was an approval or changes requested when it was submitted.
+function countsAsResponse(review) {
+  if (['APPROVED', 'CHANGES_REQUESTED', 'DISMISSED'].includes(review.state)) return true;
+  return review.state === 'COMMENTED' && (Boolean(review.body?.trim()) || review.comments > 0);
+}
+
+function targetHours(pr) {
+  return pr.additions < SMALL_PR_LINES ? TARGET_HOURS.small : TARGET_HOURS.standard;
+}
+
+function byTime(a, b) {
+  return a.at - b.at;
+}
+
+// Whether the PR was opened as a draft. GitHub records no event for the state a PR is opened in,
+// so it's read off the first ready or draft event, or the PR's current state if there are none.
+function openedAsDraft(pr) {
+  const first = pr.timeline.filter((e) => e.type === 'ready' || e.type === 'draft').sort(byTime)[0];
+  return first ? first.type === 'ready' : pr.isDraft;
+}
+
+function rfcClock(pr, until) {
+  const reviews = pr.timeline
+    .filter((e) => e.type === 'review' && e.at <= until && e.author && e.author.login !== pr.author.login)
+    .sort(byTime);
+  const ready = pr.timeline.filter((e) => e.type === 'ready' && e.at <= until).sort(byTime)[0];
+  const start = ready ? ready.at : pr.createdAt;
+  if (start > until) return null;
+  const first = reviews[0];
+  if (!first) return { start, reviewedAt: null, hours: null };
+  if (first.at < start) return { start, reviewedAt: first.at, hours: null, untimed: 'reviewed before ready' };
+  return { start, reviewedAt: first.at, hours: (first.at - start) / HOUR_MS };
+}
+
+// status: 'responded', 'waiting' (still ready and unreviewed at `until`), 'closed' (closed
+// unreviewed), 'reviewed while draft', or 'not ready' (a draft at `until`, or never ready).
+function slaClock(pr, until, holidays) {
+  const target = targetHours(pr);
+  const end = pr.closedAt && pr.closedAt <= until ? pr.closedAt : until;
+  let start = openedAsDraft(pr) ? null : pr.createdAt;
+  const events = pr.timeline.filter((e) => e.at <= end).sort(byTime);
+  for (const event of events) {
+    if (event.type === 'ready') start ??= event.at;
+    else if (event.type === 'draft') start = null;
+    else if (event.type === 'review' && !isBot(event.author) && event.author.login !== pr.author.login && countsAsResponse(event)) {
+      if (!start) return { status: 'reviewed while draft', target };
+      const hours = businessHoursBetween(start, event.at, holidays);
+      return { status: 'responded', start, reviewedAt: event.at, reviewer: event.author.login, hours, target, met: hours <= target };
+    }
+  }
+  if (!start) return { status: 'not ready', target };
+  // Still unreviewed: it's a miss once it's past the target, and not yet known before that.
+  const hours = businessHoursBetween(start, end, holidays);
+  const status = end === until ? 'waiting' : 'closed';
+  return { status, start, reviewedAt: null, hours, target, met: hours > target ? false : null };
+}
+
+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 };