ReviewSLA: keep standing approvals across drafts; reassign overdue reviews

ReviewSLA/README.md

@@ -34,7 +34,8 @@ Each assignee other than the author has their own clock on each ready PR:
   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.
+  over, except for anyone whose approval still stands: they owe nothing until a re-request after a
+  push.
 
 Bot PRs and Graphite merge-queue PRs have no clocks.
 
@@ -47,7 +48,7 @@ Bot PRs and Graphite merge-queue PRs have no clocks.
 - **Reminder DMs** (`remind`): one per clock, when it's due, to the assignee's Slack ID in
   `REVIEW_PEOPLE`, linking to the PR in Graphite.
 - **Out-of-office reassignment** (`remind`): an assignee the PTO calendar has out on any day from
-  today to the day their review is due is replaced on every PR in the stack they're assigned to
+  today to the day their review is due (or today, for an overdue review) 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

ReviewSLA/owed.js

@@ -14,6 +14,9 @@
 // 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.
+//
+// Going back to draft and ready again starts the first-response clocks over, except for an
+// approval that still stands: that's a response whenever it was given, here and in reassign().
 
 const { addBusinessHours, businessHoursBetween } = require('../ReviewConfig/hours.js');
 const { byTime, countsAsResponse, hasSla, isBot, openedAsDraft, targetHours, TARGET_HOURS } = require('./clock.js');
@@ -33,11 +36,17 @@ 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) {
+// `login`'s latest approval or changes-requested review, if it's an approval. A dismissed
+// approval no longer counts.
+function standingApproval(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;
+  return last?.state === 'APPROVED' ? last : null;
+}
+
+// The time of `login`'s standing approval, or null.
+function approvedAt(pr, login) {
+  return standingApproval(pr, login)?.at ?? null;
 }
 
 function latest(pr, type, login) {
@@ -81,7 +90,9 @@ function clockFor(pr, login, readyAt, lower) {
   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);
+  // Reviews from before the PR was last marked ready don't count, except a standing approval.
+  const approval = standingApproval(pr, login);
+  const responses = reviewsBy(pr, login).filter((r) => countsAsResponse(r) && (r.at >= readyAt || r === approval));
   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))];

ReviewSLA/owed.test.js

@@ -78,6 +78,49 @@ test('going back to draft and ready again starts the clock over, with no miss',
   assert.deepStrictEqual([clock.start, clock.overdue], ['2026-10-05T17:00:00.000Z', false]);
 });
 
+test('an approval that still stands survives going back to draft and ready again', () => {
+  // ghopper approved while the PR was a draft, and it's been pushed to since without a re-request.
+  const p = pr({
+    headRefOid: 'bbb',
+    timeline: [
+      ['draft', '2026-10-01T18:00:00Z'],
+      ['review', '2026-10-02T17:00:00Z', { ...review('ghopper'), commit: 'aaa' }],
+      ['ready', '2026-10-05T17:00:00Z'],
+    ],
+  });
+  assert.deepStrictEqual(clocks([p], '2026-10-05T18:00:00Z').map((c) => c.login), ['kjohnson']);
+});
+
+test('after a draft round-trip, a standing approval still gets a re-review clock from a re-request after a push', () => {
+  const p = pr({
+    assignees: ['ghopper'],
+    headRefOid: 'bbb',
+    timeline: [
+      ['draft', '2026-10-01T18:00:00Z'],
+      ['review', '2026-10-02T17:00:00Z', { ...review('ghopper'), commit: 'aaa' }],
+      ['ready', '2026-10-05T17:00:00Z'],
+      ['requested', '2026-10-05T17:30:00Z', { login: 'ghopper' }],
+    ],
+  });
+  assert.deepStrictEqual(clocks([p], '2026-10-05T18:00:00Z').map((c) => [c.kind, c.start]), [['re-review', '2026-10-05T17:30:00.000Z']]);
+});
+
+test('a comment, or an approval since replaced by a change request, before going back to draft doesn\'t carry over', () => {
+  const p = pr({
+    timeline: [
+      ['review', '2026-10-01T17:30:00Z', review('ghopper')],
+      ['review', '2026-10-01T17:45:00Z', review('ghopper', 'CHANGES_REQUESTED')],
+      ['review', '2026-10-01T17:50:00Z', review('kjohnson', 'COMMENTED')],
+      ['draft', '2026-10-01T18:00:00Z'],
+      ['ready', '2026-10-05T17:00:00Z'],
+    ],
+  });
+  assert.deepStrictEqual(clocks([p], '2026-10-05T18:00:00Z').map((c) => [c.login, c.start]), [
+    ['ghopper', '2026-10-05T17:00:00.000Z'],
+    ['kjohnson', '2026-10-05T17:00:00.000Z'],
+  ]);
+});
+
 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');

ReviewSLA/run.js

@@ -61,11 +61,14 @@ function isDigestRun(inputs, now) {
   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.
+// Whether the calendar has `login` out on any day from today to the day the review is due, or
+// today if it's already overdue.
 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;
+  const today = pacificDate(now);
+  const last = pacificDate(due) > today ? pacificDate(due) : today;
+  for (let date = today; date <= last; date = addDays(date, 1)) if (dates.has(date)) return true;
   return false;
 }
 

ReviewSLA/run.test.js

@@ -235,6 +235,20 @@ test('on a public repo, the reassignment notice doesn\'t say the person is out',
   assert.doesNotMatch(notice.body, /PTO|out/);
 });
 
+test('an overdue review is reassigned when its assignee is out today', async () => {
+  // Due 17:00 Thursday Oct 1; on Monday Oct 5 ghopper is out and still hasn't responded.
+  const w = world({ prs: [onePr()], out: [['Person ghopper', '2026-10-05']] });
+  await go(w, { now: '2026-10-05T18:00:00Z' });
+  assert.deepStrictEqual(w.calls.unassign, [[1, 'ghopper']]);
+  assert.deepStrictEqual(w.calls.assign, [[1, 'mhamilton']]);
+});
+
+test('an overdue review stays put when its assignee was only out before today', async () => {
+  const w = world({ prs: [onePr()], out: [['Person ghopper', '2026-10-02']] });
+  await go(w, { now: '2026-10-05T18:00:00Z' });
+  assert.deepStrictEqual(w.calls.assign, []);
+});
+
 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' });