diff --git a/.github/workflows/assign-reviewers-tests.yml b/.github/workflows/review-tests.yml similarity index 53% rename from .github/workflows/assign-reviewers-tests.yml rename to .github/workflows/review-tests.yml index 51caafc..31391bf 100644 --- a/.github/workflows/assign-reviewers-tests.yml +++ b/.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/ diff --git a/.gitignore b/.gitignore index 758116a..00e0f8f 100644 --- a/.gitignore +++ b/.gitignore @@ -1,2 +1,3 @@ .DS_Store ci-metrics-data/ +review-sla-report/ diff --git a/ReviewConfig/README.md b/ReviewConfig/README.md index 11fa52b..dd80e0a 100644 --- a/ReviewConfig/README.md +++ b/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 diff --git a/ReviewConfig/hours.js b/ReviewConfig/hours.js new file mode 100644 index 0000000..c14284c --- /dev/null +++ b/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 }; diff --git a/ReviewConfig/hours.test.js b/ReviewConfig/hours.test.js new file mode 100644 index 0000000..5dad244 --- /dev/null +++ b/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); +}); diff --git a/ReviewConfig/pto.js b/ReviewConfig/pto.js index 4393213..c335521 100644 --- a/ReviewConfig/pto.js +++ b/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: { "": [{ "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, +}; diff --git a/ReviewSLA/README.md b/ReviewSLA/README.md new file mode 100644 index 0000000..dac75fc --- /dev/null +++ b/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__.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/ +``` diff --git a/ReviewSLA/clock.js b/ReviewSLA/clock.js new file mode 100644 index 0000000..788b103 --- /dev/null +++ b/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 }; diff --git a/ReviewSLA/clock.test.js b/ReviewSLA/clock.test.js new file mode 100644 index 0000000..0fe35de --- /dev/null +++ b/ReviewSLA/clock.test.js @@ -0,0 +1,158 @@ +// Run with: node --test ReviewSLA/ +const test = require('node:test'); +const assert = require('node:assert'); +const { countsAsResponse, firstResponse, hasSla } = require('./clock.js'); +const { measure, percentile, toPr, weekOf } = require('./report.js'); + +const HOLIDAYS = { 2026: [{ date: '2026-11-26' }, { date: '2026-11-27' }] }; +const UNTIL = new Date('2026-12-31T00:00:00Z'); +const at = (iso) => new Date(iso); + +const author = { login: 'alovelace', bot: false }; +const reviewer = { login: 'ghopper', bot: false }; + +function pr(overrides = {}) { + return { + number: 1, + author, + createdAt: at('2026-10-01T17:00:00Z'), // 10:00 Thursday, Pacific + closedAt: null, + isDraft: false, + additions: 100, + headRefName: 'feature/x', + timeline: [], + ...overrides, + }; +} +const ready = (iso) => ({ type: 'ready', at: at(iso) }); +const draft = (iso) => ({ type: 'draft', at: at(iso) }); +const review = (iso, state = 'APPROVED', extra = {}) => ({ type: 'review', at: at(iso), author: reviewer, state, body: '', comments: 0, ...extra }); +const clock = (p, until = UNTIL) => firstResponse(p, { until, holidays: HOLIDAYS }); + +test('reviews that count: approvals, changes requested, and comments with something in them', () => { + assert.ok(countsAsResponse({ state: 'APPROVED' })); + assert.ok(countsAsResponse({ state: 'CHANGES_REQUESTED' })); + assert.ok(countsAsResponse({ state: 'DISMISSED' })); + assert.ok(countsAsResponse({ state: 'COMMENTED', body: 'Looks close', comments: 0 })); + assert.ok(countsAsResponse({ state: 'COMMENTED', body: '', comments: 2 })); + assert.ok(!countsAsResponse({ state: 'COMMENTED', body: ' ', comments: 0 })); +}); + +test('a small PR marked ready at 16:30 Friday and reviewed at 13:30 Monday is just within target', () => { + const p = pr({ + createdAt: at('2026-10-02T20:00:00Z'), + timeline: [ready('2026-10-02T23:30:00Z'), review('2026-10-05T20:30:00Z')], + }); + const { sla, rfc } = clock(p); + assert.deepStrictEqual([sla.status, sla.hours, sla.target, sla.met, sla.reviewer], ['responded', 4, 4, true, 'ghopper']); + assert.strictEqual(rfc.hours, 69); +}); + +test('a standard PR gets one business day', () => { + const p = pr({ additions: 250, timeline: [review('2026-10-02T17:00:00Z')] }); + assert.deepStrictEqual([clock(p).sla.hours, clock(p).sla.target, clock(p).sla.met], [7, 7, true]); +}); + +test('the day before a listed holiday, the holiday doesn\'t count', () => { + // Ready 16:00 Wednesday before Thanksgiving, reviewed 11:00 the Monday after. + const p = pr({ createdAt: at('2026-11-26T00:00:00Z'), timeline: [review('2026-11-30T19:00:00Z')] }); + assert.strictEqual(clock(p).sla.hours, 2); +}); + +test('only the author\'s own reviews, bots and empty comment reviews leave the clock running', () => { + const p = pr({ + timeline: [ + review('2026-10-01T18:00:00Z', 'COMMENTED', { author, body: 'Note to self' }), + review('2026-10-01T18:30:00Z', 'COMMENTED', { author: { login: 'copilot', bot: true }, body: 'Summary' }), + review('2026-10-01T19:00:00Z', 'COMMENTED'), + review('2026-10-01T21:00:00Z', 'COMMENTED', { comments: 1 }), + ], + }); + assert.deepStrictEqual([clock(p).sla.status, clock(p).sla.hours], ['responded', 4]); +}); + +test('going back to draft stops the clock, and marking ready again starts it over', () => { + const p = pr({ + timeline: [draft('2026-10-01T22:00:00Z'), ready('2026-10-05T17:00:00Z'), review('2026-10-05T19:00:00Z')], + }); + const { sla, rfc } = clock(p); + assert.deepStrictEqual([sla.start, sla.hours], [at('2026-10-05T17:00:00Z'), 2]); + // The RFC measured from the first ready time, which for a PR opened ready is when it was opened. + assert.deepStrictEqual(rfc.start, at('2026-10-05T17:00:00Z')); +}); + +test('a PR opened as a draft starts its clock when it\'s marked ready', () => { + const p = pr({ isDraft: false, timeline: [ready('2026-10-02T17:00:00Z'), review('2026-10-02T18:00:00Z')] }); + assert.deepStrictEqual([clock(p).sla.start, clock(p).sla.hours], [at('2026-10-02T17:00:00Z'), 1]); +}); + +test('a PR reviewed while still a draft isn\'t timed', () => { + const p = pr({ isDraft: true, timeline: [review('2026-10-02T18:00:00Z'), ready('2026-10-05T17:00:00Z')] }); + assert.strictEqual(clock(p).sla.status, 'reviewed while draft'); + assert.strictEqual(clock(p).rfc.untimed, 'reviewed before ready'); +}); + +test('an unreviewed PR is a miss once it\'s past its target, and unknown before that', () => { + const p = pr(); + const early = clock(p, at('2026-10-01T19:00:00Z')).sla; + assert.deepStrictEqual([early.status, early.hours, early.met], ['waiting', 2, null]); + const late = clock(p, at('2026-10-02T17:00:00Z')).sla; + assert.deepStrictEqual([late.status, late.hours, late.met], ['waiting', 7, false]); +}); + +test('a PR closed unreviewed stops its clock when it closed', () => { + const p = pr({ closedAt: at('2026-10-01T18:00:00Z') }); + assert.deepStrictEqual([clock(p).sla.status, clock(p).sla.hours, clock(p).sla.met], ['closed', 1, null]); +}); + +test('events after the end of the range are ignored', () => { + // Opened as a draft, reviewed, and marked ready after the range ended: the RFC timed it from creation. + const p = pr({ isDraft: false, timeline: [review('2026-10-02T18:00:00Z'), ready('2026-10-09T17:00:00Z')] }); + const { rfc, sla } = clock(p, at('2026-10-05T00:00:00Z')); + assert.deepStrictEqual([rfc.start, rfc.hours], [p.createdAt, 25]); + assert.strictEqual(sla.status, 'reviewed while draft'); + const unreviewed = clock(pr({ timeline: [review('2026-10-09T18:00:00Z')] }), at('2026-10-05T00:00:00Z')); + assert.deepStrictEqual([unreviewed.rfc.reviewedAt, unreviewed.sla.status], [null, 'waiting']); +}); + +test('bot, Graphite and merge-queue PRs have no SLA', () => { + assert.ok(hasSla(pr())); + assert.ok(!hasSla(pr({ author: { login: 'dependabot[bot]', bot: true } }))); + assert.ok(!hasSla(pr({ author: { login: 'graphite-app', bot: false } }))); + assert.ok(!hasSla(pr({ headRefName: 'gtmq_spec_1234' }))); +}); + +test('report: percentiles interpolate, weeks start on Monday', () => { + assert.strictEqual(percentile([1, 2, 3, 4], 0.5), 2.5); + assert.strictEqual(percentile([10], 0.9), 10); + assert.strictEqual(percentile([], 0.5), null); + assert.strictEqual(weekOf('2026-10-01'), '2026-09-28'); + assert.strictEqual(weekOf('2026-10-04'), '2026-09-28'); + assert.strictEqual(weekOf('2026-10-05'), '2026-10-05'); +}); + +test('report: GraphQL nodes become PRs, and only clocks started in range are counted', () => { + const node = { + number: 7, + url: 'https://github.com/o/r/pull/7', + createdAt: '2026-10-01T17:00:00Z', + closedAt: null, + isDraft: false, + additions: 10, + headRefName: 'feature/y', + author: { __typename: 'User', login: 'alovelace' }, + timelineItems: { + pageInfo: { hasNextPage: false }, + nodes: [ + { __typename: 'PullRequestReview', state: 'PENDING', submittedAt: null, body: '', comments: { totalCount: 0 }, author: { __typename: 'User', login: 'ghopper' } }, + { __typename: 'PullRequestReview', state: 'APPROVED', submittedAt: '2026-10-01T18:00:00Z', body: '', comments: { totalCount: 0 }, author: { __typename: 'User', login: 'ghopper' } }, + ], + }, + }; + const converted = toPr(node); + assert.strictEqual(converted.timeline.length, 1); + const [row] = measure([converted], { since: '2026-10-01', until: '2026-10-01', holidays: HOLIDAYS }); + assert.deepStrictEqual([row.rfc.hours, row.sla.hours, row.slaInRange], [1, 1, true]); + const [outside] = measure([converted], { since: '2026-10-02', until: '2026-10-08', holidays: HOLIDAYS }); + assert.deepStrictEqual([outside.rfc, outside.slaInRange], [null, false]); +}); diff --git a/ReviewSLA/report.js b/ReviewSLA/report.js new file mode 100644 index 0000000..7f3c668 --- /dev/null +++ b/ReviewSLA/report.js @@ -0,0 +1,264 @@ +#!/usr/bin/env node +// Report mode: weekly first-response numbers for a date range (review RFC, Proposal 2). +// +// node ReviewSLA/report.js StaflLib coit-tower-bms2000 --since 2026-06-26 --until 2026-09-24 +// +// Writes review_sla__.md and a prs.tsv with one row per PR to --out. Each PR counts +// in the week (Monday to Sunday, Pacific) its clock started, and events after --until are ignored, +// so a past range reads the same however long after it the report is run. + +const { execFileSync } = require('node:child_process'); +const fs = require('node:fs'); +const path = require('node:path'); +const { parseArgs } = require('node:util'); +const { addDays, pacificDate, readHolidays } = require('../ReviewConfig/pto.js'); +const { pacificTime } = require('../ReviewConfig/hours.js'); +const { firstResponse, hasSla, SMALL_PR_LINES, TARGET_HOURS } = require('./clock.js'); + +// PRs created this long before --since are still fetched, in case they were marked ready in range. +const LOOKBACK_DAYS = 60; + +const PR_QUERY = `query($owner: String!, $name: String!, $after: String) { + repository(owner: $owner, name: $name) { + pullRequests(first: 50, after: $after, orderBy: { field: CREATED_AT, direction: DESC }) { + pageInfo { hasNextPage endCursor } + nodes { + number url createdAt closedAt isDraft additions headRefName + author { __typename login } + timelineItems(first: 100, itemTypes: [READY_FOR_REVIEW_EVENT, CONVERT_TO_DRAFT_EVENT, PULL_REQUEST_REVIEW]) { + pageInfo { hasNextPage } + nodes { + __typename + ... on ReadyForReviewEvent { createdAt } + ... on ConvertToDraftEvent { createdAt } + ... on PullRequestReview { state submittedAt body comments { totalCount } author { __typename login } } + } + } + } + } + } +}`; + +const actor = (a) => a && { login: a.login, bot: a.__typename === 'Bot' }; + +// GraphQL nodes to the PR shape clock.js reads. Pending reviews (no submittedAt) are dropped. +// Only the first 100 timeline items are read. First responses come early, so that's almost always +// enough; a PR with more and no response in the first 100 is flagged as truncated. +function toPr(node) { + const timeline = []; + for (const item of node.timelineItems.nodes) { + if (item.__typename === 'ReadyForReviewEvent') timeline.push({ type: 'ready', at: new Date(item.createdAt) }); + else if (item.__typename === 'ConvertToDraftEvent') timeline.push({ type: 'draft', at: new Date(item.createdAt) }); + else if (item.__typename === 'PullRequestReview' && item.submittedAt) { + timeline.push({ + type: 'review', + at: new Date(item.submittedAt), + author: actor(item.author), + state: item.state, + body: item.body, + comments: item.comments.totalCount, + }); + } + } + return { + number: node.number, + url: node.url, + author: actor(node.author) ?? { login: 'ghost', bot: false }, + createdAt: new Date(node.createdAt), + closedAt: node.closedAt ? new Date(node.closedAt) : null, + isDraft: node.isDraft, + additions: node.additions, + headRefName: node.headRefName, + truncated: node.timelineItems.pageInfo.hasNextPage, + timeline, + }; +} + +async function fetchPrs({ graphql, owner, repo, since }) { + const oldest = pacificTime(addDays(since, -LOOKBACK_DAYS), 0); + const prs = []; + for (let after = null; ; ) { + const data = await graphql(PR_QUERY, { owner, name: repo, after }); + const page = data.repository.pullRequests; + for (const node of page.nodes) prs.push(toPr(node)); + const last = page.nodes.at(-1); + if (!page.pageInfo.hasNextPage || !last || new Date(last.createdAt) < oldest) break; + after = page.pageInfo.endCursor; + } + return prs; +} + +// Percentile by linear interpolation between closest ranks. +function percentile(values, p) { + if (values.length === 0) return null; + const sorted = [...values].sort((a, b) => a - b); + const rank = (sorted.length - 1) * p; + const low = Math.floor(rank); + return sorted[low] + (sorted[Math.min(low + 1, sorted.length - 1)] - sorted[low]) * (rank - low); +} + +// The Monday of the week a date is in. +function weekOf(date) { + const day = new Date(`${date}T12:00:00Z`).getUTCDay(); + return addDays(date, -((day + 6) % 7)); +} + +function summarize(rows) { + const rfcHours = rows.filter((r) => r.rfc?.hours != null).map((r) => r.rfc.hours); + const responded = rows.filter((r) => r.sla.status === 'responded'); + const known = rows.filter((r) => r.sla.met != null); + return { + rfc: { + timed: rfcHours.length, + median: percentile(rfcHours, 0.5), + p90: percentile(rfcHours, 0.9), + within8: rfcHours.length ? rfcHours.filter((h) => h <= 8).length / rfcHours.length : null, + }, + sla: { + responded: responded.length, + median: percentile(responded.map((r) => r.sla.hours), 0.5), + p90: percentile(responded.map((r) => r.sla.hours), 0.9), + withinTarget: known.length ? known.filter((r) => r.sla.met).length / known.length : null, + overdue: rows.filter((r) => r.sla.met === false && r.sla.status !== 'responded').length, + }, + }; +} + +// Every PR with an SLA, with both clocks, and the dates each clock started (null if it never did). +function measure(prs, { since, until, holidays }) { + const end = pacificTime(addDays(until, 1), 0); + const inRange = (start) => start && pacificDate(start) >= since && pacificDate(start) <= until; + return prs.filter(hasSla).map((pr) => { + const { rfc, sla } = firstResponse(pr, { until: end, holidays }); + return { + pr, + rfc: inRange(rfc?.start) ? rfc : null, + sla, + slaInRange: inRange(sla.start), + }; + }); +} + +const fmt = (h) => (h == null ? '—' : `${h.toFixed(1)}`); +const pct = (f) => (f == null ? '—' : `${Math.round(f * 100)}%`); + +function table(measured) { + const weeks = new Map(); + const add = (week, kind, row) => { + if (!weeks.has(week)) weeks.set(week, { rfc: [], sla: [] }); + weeks.get(week)[kind].push(row); + }; + for (const row of measured) { + if (row.rfc) add(weekOf(pacificDate(row.rfc.start)), 'rfc', row); + if (row.slaInRange) add(weekOf(pacificDate(row.sla.start)), 'sla', row); + } + const lines = [ + '| Week of | RFC: timed | median h | p90 h | within 8 h | SLA: responded | median bh | p90 bh | within target | overdue, unreviewed |', + '| --- | ---: | ---: | ---: | ---: | ---: | ---: | ---: | ---: | ---: |', + ]; + const line = (label, rfcRows, slaRows) => { + const { rfc } = summarize(rfcRows); + const { sla } = summarize(slaRows); + lines.push( + `| ${label} | ${rfc.timed} | ${fmt(rfc.median)} | ${fmt(rfc.p90)} | ${pct(rfc.within8)} | ` + + `${sla.responded} | ${fmt(sla.median)} | ${fmt(sla.p90)} | ${pct(sla.withinTarget)} | ${sla.overdue} |`, + ); + }; + for (const week of [...weeks.keys()].sort()) line(week, weeks.get(week).rfc, weeks.get(week).sla); + line( + '**All**', + measured.filter((r) => r.rfc), + measured.filter((r) => r.slaInRange), + ); + return lines.join('\n'); +} + +function renderReport(byRepo, { since, until }) { + const parts = [ + `# First response, ${since} to ${until}`, + '', + 'RFC columns measure as the RFC did: wall-clock hours from the first ready event (or creation) to the first', + 'review by anyone but the author, leaving out PRs reviewed before that. SLA columns follow the SLA clock rules:', + `business hours (10:00 to 17:00 Pacific, business days) from the latest time the PR was marked ready, to the first`, + `review that counts. The target is ${TARGET_HOURS.small} h under ${SMALL_PR_LINES} added lines and ${TARGET_HOURS.standard} h otherwise. "Within target" also counts`, + 'PRs still unreviewed past their target as misses. Weeks start on Monday, by when each clock started.', + ]; + for (const [repo, measured] of Object.entries(byRepo)) { + const untimed = measured.filter((r) => r.sla.status === 'reviewed while draft').length; + parts.push('', `## ${repo}`, '', table(measured)); + if (untimed) parts.push('', `${untimed} PRs were reviewed while still drafts, so the SLA columns don't time them.`); + } + return `${parts.join('\n')}\n`; +} + +function tsv(byRepo) { + const iso = (d) => (d ? d.toISOString() : ''); + const num = (h) => (h == null ? '' : h.toFixed(2)); + const rows = [['repo', 'pr', 'author', 'additions', 'rfc_start', 'rfc_first_review', 'rfc_hours', 'sla_status', 'sla_start', 'sla_first_response', 'reviewer', 'business_hours', 'target', 'met']]; + for (const [repo, measured] of Object.entries(byRepo)) { + for (const { pr, rfc, sla, slaInRange } of measured) { + if (!rfc && !slaInRange) continue; + rows.push([repo, pr.number, pr.author.login, pr.additions, iso(rfc?.start), iso(rfc?.reviewedAt), num(rfc?.hours), sla.status, + iso(sla.start), iso(sla.reviewedAt), sla.reviewer ?? '', num(sla.hours), sla.target, sla.met ?? '']); + } + } + return `${rows.map((r) => r.join('\t')).join('\n')}\n`; +} + +function githubGraphql(token) { + return async (query, variables) => { + const response = await fetch('https://api.github.com/graphql', { + method: 'POST', + headers: { authorization: `bearer ${token}`, 'content-type': 'application/json' }, + body: JSON.stringify({ query, variables }), + }); + const body = await response.json(); + if (!response.ok || body.errors) throw new Error(`GraphQL: ${JSON.stringify(body.errors ?? body)}`); + return body.data; + }; +} + +async function main() { + const { values, positionals } = parseArgs({ + allowPositionals: true, + options: { + since: { type: 'string' }, + until: { type: 'string' }, + owner: { type: 'string', default: 'StaflSystems' }, + out: { type: 'string', default: 'review-sla-report' }, + }, + }); + const { since, until, owner, out } = values; + if (!since || !until || positionals.length === 0) { + console.error('Usage: node ReviewSLA/report.js ... --since YYYY-MM-DD --until YYYY-MM-DD [--owner org] [--out dir]'); + process.exit(2); + } + const token = process.env.GITHUB_TOKEN || execFileSync('gh', ['auth', 'token'], { encoding: 'utf8' }).trim(); + const graphql = githubGraphql(token); + const holidays = readHolidays(); + const byRepo = {}; + for (const repo of positionals) { + const prs = await fetchPrs({ graphql, owner, repo, since }); + byRepo[repo] = measure(prs, { since, until, holidays }); + for (const { pr, sla } of byRepo[repo]) { + if (pr.truncated && sla.status !== 'responded') { + console.error(`${repo}#${pr.number}: more than 100 timeline items and no response in the first 100; its SLA clock may be wrong.`); + } + } + console.error(`${repo}: ${prs.length} PRs fetched.`); + } + fs.mkdirSync(out, { recursive: true }); + const report = renderReport(byRepo, { since, until }); + fs.writeFileSync(path.join(out, `review_sla_${since}_${until}.md`), report); + fs.writeFileSync(path.join(out, 'prs.tsv'), tsv(byRepo)); + process.stdout.write(report); +} + +if (require.main === module) { + main().catch((error) => { + console.error(error.message); + process.exit(1); + }); +} + +module.exports = { measure, percentile, renderReport, toPr, weekOf };