From 630fc2682a5bd39b25a8e61f74b6b3f14f2b1e21 Mon Sep 17 00:00:00 2001 From: Paul Bakaus Date: Fri, 10 Jul 2026 12:02:52 -0700 Subject: [PATCH] Base sheriff stale clock on blocker age (#364) --- scripts/github/sheriff.mjs | 118 +++++++++++++--- tests/github-sheriff.test.mjs | 246 +++++++++++++++++++++++++++++++++- 2 files changed, 342 insertions(+), 22 deletions(-) diff --git a/scripts/github/sheriff.mjs b/scripts/github/sheriff.mjs index e4539eefd..07b46e527 100644 --- a/scripts/github/sheriff.mjs +++ b/scripts/github/sheriff.mjs @@ -38,6 +38,7 @@ export const CLOSE_MARKER = ''; const DEFAULT_MAINTAINERS = ['pbakaus']; const DEFAULT_REGULAR_CONTRIBUTORS = ['pbakaus', 'abdulwahabone']; const DEFAULT_EXEMPT_LABELS = ['do not close', 'security']; +const DEFAULT_TRUSTED_MARKER_AUTHORS = ['github-actions', 'github-actions[bot]']; const REVIEW_BLOCKING_STATES = new Set(['CHANGES_REQUESTED']); const FAILING_STATUS_STATES = new Set(['ERROR', 'FAILURE']); @@ -59,6 +60,8 @@ query($owner: String!, $name: String!, $after: String) { reviewDecision author { login } labels(first: 50) { nodes { name } } + # This is only an initial seed; hydrateIssueComments() replaces it with + # the full paginated issue comment history before evaluation. comments(last: 50) { nodes { author { login } @@ -97,7 +100,7 @@ query($owner: String!, $name: String!, $after: String) { } } } - timelineItems(last: 50, itemTypes: [LABELED_EVENT, UNLABELED_EVENT]) { + labelTimelineItems: timelineItems(last: 100, itemTypes: [LABELED_EVENT, UNLABELED_EVENT]) { nodes { __typename ... on LabeledEvent { @@ -112,6 +115,19 @@ query($owner: String!, $name: String!, $after: String) { } } } + draftTimelineItems: timelineItems(last: 100, itemTypes: [CONVERT_TO_DRAFT_EVENT, READY_FOR_REVIEW_EVENT]) { + nodes { + __typename + ... on ConvertToDraftEvent { + createdAt + actor { login } + } + ... on ReadyForReviewEvent { + createdAt + actor { login } + } + } + } } } } @@ -124,6 +140,7 @@ export function evaluatePullRequest(pr, options = {}) { const maintainers = loginSet(options.maintainers || DEFAULT_MAINTAINERS); const regularContributors = loginSet(options.regularContributors || DEFAULT_REGULAR_CONTRIBUTORS); const exemptLabels = new Set(options.exemptLabels || DEFAULT_EXEMPT_LABELS); + const trustedMarkerAuthors = loginSet(options.trustedMarkerAuthors || DEFAULT_TRUSTED_MARKER_AUTHORS); const autoCloseRegulars = options.autoCloseRegulars === true; const labels = new Set(pr.labels || []); @@ -148,7 +165,7 @@ export function evaluatePullRequest(pr, options = {}) { const addBlocker = (blocker) => blockers.push({ contributorAction: false, ...blocker }); const addContributorBlocker = (blocker) => blockers.push({ contributorAction: true, ...blocker }); if (pr.isDraft) { - addContributorBlocker({ kind: 'draft' }); + addContributorBlocker({ kind: 'draft', at: currentDraftStartedAt(pr) }); } if (FAILING_STATUS_STATES.has(pr.statusState)) { @@ -185,7 +202,13 @@ export function evaluatePullRequest(pr, options = {}) { addContributorBlocker({ kind: 'manual-waiting', at: waitingLabelAt }); } + const contributorActionBlockerAt = latestDate(blockers + .filter((blocker) => blocker.contributorAction) + .map((blocker) => blocker.at || pr.createdAt)); const contributorActionRequired = blockers.some((blocker) => blocker.contributorAction); + const waitingDays = contributorActionRequired && contributorActionBlockerAt + ? Math.floor((now.getTime() - contributorActionBlockerAt.getTime()) / DAY_MS) + : 0; const unresolvedThreadCount = (pr.reviewThreads || []).filter((thread) => !thread.isResolved).length; const statusIsReady = pr.statusState === 'SUCCESS'; const mergeableIsReady = pr.mergeable === 'MERGEABLE'; @@ -205,16 +228,18 @@ export function evaluatePullRequest(pr, options = {}) { if (blocker.label) desiredLabels.add(blocker.label); } - const staleEligible = contributorActionRequired && daysOpen >= warningDays; + const staleEligible = contributorActionRequired && waitingDays >= warningDays; if (staleEligible) desiredLabels.add('stale'); - const warningAlreadyPosted = labels.has('stale') || hasMarker(pr.comments, WARNING_MARKER); - const closeAlreadyPosted = hasMarker(pr.comments, CLOSE_MARKER); + const warningPostedAt = latestMarkerAt(pr.comments, WARNING_MARKER, trustedMarkerAuthors); + const warningAlreadyPosted = Boolean(warningPostedAt + && (!contributorActionBlockerAt || !isAfter(contributorActionBlockerAt, warningPostedAt))); + const closeAlreadyPosted = hasMarker(pr.comments, CLOSE_MARKER, trustedMarkerAuthors); const exemptFromClose = [...labels].some((label) => exemptLabels.has(label)); const regularContributor = regularContributors.has(author); const shouldWarn = staleEligible && !warningAlreadyPosted; const shouldClose = contributorActionRequired - && daysOpen >= closeDays + && waitingDays >= closeDays && warningAlreadyPosted && !closeAlreadyPosted && !exemptFromClose @@ -230,6 +255,7 @@ export function evaluatePullRequest(pr, options = {}) { title: pr.title, author, daysOpen, + waitingDays, contributorActionRequired, readyToMerge, blockers: blockers.map((blocker) => blocker.kind), @@ -238,13 +264,16 @@ export function evaluatePullRequest(pr, options = {}) { labelsToRemove, shouldWarn, shouldClose, - warningComment: shouldWarn ? staleWarningComment(pr, { daysOpen, closeDays, now }) : '', - closeComment: shouldClose ? staleCloseComment(pr, { daysOpen }) : '', + warningComment: shouldWarn ? staleWarningComment(pr, { waitingDays, closeDays, contributorActionBlockerAt, now }) : '', + closeComment: shouldClose ? staleCloseComment(pr, { waitingDays }) : '', }; } export function normalizePullRequest(node) { const latestCommit = node.commits?.nodes?.[0]?.commit || null; + const timelineNodes = node.timelineItems?.nodes || []; + const labelTimelineNodes = node.labelTimelineItems?.nodes || timelineNodes; + const draftTimelineNodes = node.draftTimelineItems?.nodes || timelineNodes; return { number: node.number, title: node.title, @@ -271,7 +300,7 @@ export function normalizePullRequest(node) { isResolved: thread.isResolved, comments: (thread.comments?.nodes || []).map(normalizeComment), })), - labelEvents: (node.timelineItems?.nodes || []) + labelEvents: labelTimelineNodes .filter((event) => event.label?.name) .map((event) => ({ type: event.__typename, @@ -279,6 +308,13 @@ export function normalizePullRequest(node) { actorLogin: event.actor?.login || '', createdAt: event.createdAt, })), + draftEvents: draftTimelineNodes + .filter((event) => ['ConvertToDraftEvent', 'ReadyForReviewEvent'].includes(event.__typename)) + .map((event) => ({ + type: event.__typename, + actorLogin: event.actor?.login || '', + createdAt: event.createdAt, + })), }; } @@ -317,8 +353,18 @@ export function mergeIssueLabelEvents(pr, events = []) { return pr; } -export function staleWarningComment(pr, { daysOpen, closeDays, now = new Date() }) { - const scheduledCloseAt = addDays(toDate(pr.createdAt), closeDays); +export function mergeIssueComments(pr, comments = []) { + pr.comments = comments.map(normalizeComment); + return pr; +} + +export function staleWarningComment(pr, { + waitingDays, + closeDays, + contributorActionBlockerAt, + now = new Date(), +}) { + const scheduledCloseAt = addDays(toDate(contributorActionBlockerAt || now), closeDays); const earliestNewWarningCloseAt = addDays(toDate(now), 1); const closeDate = formatDate(scheduledCloseAt > earliestNewWarningCloseAt ? scheduledCloseAt @@ -327,16 +373,16 @@ export function staleWarningComment(pr, { daysOpen, closeDays, now = new Date() WARNING_MARKER, `Thanks for the PR. Impeccable is moving quickly, and this PR is currently waiting on contributor action.`, '', - `It has been open for ${daysOpen} days. Please address the outstanding review feedback, draft state, or explicit maintainer wait request. PRs that are still waiting on contributor action after ${closeDays} days open are closed automatically.`, + `It has been waiting for contributor action for ${waitingDays} days. Please address the outstanding review feedback, draft state, or explicit maintainer wait request. PRs that are still waiting on contributor action after ${closeDays} days are closed automatically.`, '', `If nothing changes, this PR may be closed on or after ${closeDate}. Happy to reopen when it is ready to continue.`, ].join('\n'); } -export function staleCloseComment(pr, { daysOpen }) { +export function staleCloseComment(pr, { waitingDays }) { return [ CLOSE_MARKER, - `Closing this because it has been open for ${daysOpen} days and is still waiting on contributor action.`, + `Closing this because it has been waiting on contributor action for ${waitingDays} days.`, '', 'Please open a fresh PR, or ask for this one to be reopened, after the outstanding feedback is addressed.', ].join('\n'); @@ -354,6 +400,7 @@ export async function main(argv = process.argv.slice(2)) { } const prs = fetchOpenPullRequests(repo).map(normalizePullRequest); + hydrateIssueComments(repo, prs); hydrateMissingWaitingLabelEvents(repo, prs); const plans = prs.map((pr) => evaluatePullRequest(pr, options)); @@ -423,6 +470,13 @@ function latestMaintainerWaitCommand(pr, maintainers) { ]); } +function currentDraftStartedAt(pr) { + const latestTransition = [...(pr.draftEvents || [])] + .filter((event) => ['ConvertToDraftEvent', 'ReadyForReviewEvent'].includes(event.type)) + .sort((a, b) => toDate(b.createdAt) - toDate(a.createdAt))[0]; + return latestTransition?.createdAt || pr.createdAt; +} + function hasSheriffWaitCommand(body) { return String(body || '') .split(/\r?\n/) @@ -462,6 +516,22 @@ function hydrateMissingWaitingLabelEvents(repo, prs) { } } +function hydrateIssueComments(repo, prs) { + for (const pr of prs) { + mergeIssueComments(pr, fetchIssueComments(repo, pr.number)); + } +} + +function fetchIssueComments(repo, number) { + const pages = runGhJson([ + 'api', + '--paginate', + '--slurp', + `repos/${repo}/issues/${number}/comments?per_page=100`, + ]); + return Array.isArray(pages) ? pages.flat() : []; +} + function fetchIssueEvents(repo, number) { const pages = runGhJson([ 'api', @@ -586,8 +656,8 @@ function printPlan(plan, { apply }) { function normalizeComment(comment) { return { - authorLogin: comment.author?.login || '', - createdAt: comment.createdAt, + authorLogin: comment.authorLogin || comment.author?.login || comment.user?.login || '', + createdAt: comment.createdAt || comment.created_at, body: comment.body || '', }; } @@ -612,8 +682,20 @@ function reviewThreadCommentsBy(threads = [], login) { return threads.flatMap((thread) => commentsBy(thread.comments, login)); } -function hasMarker(comments = [], marker) { - return comments.some((comment) => typeof comment.body === 'string' && comment.body.includes(marker)); +function hasMarker(comments = [], marker, trustedAuthors = null) { + return comments.some((comment) => isTrustedMarkerComment(comment, marker, trustedAuthors)); +} + +function latestMarkerAt(comments = [], marker, trustedAuthors = null) { + return latestDate(comments + .filter((comment) => isTrustedMarkerComment(comment, marker, trustedAuthors)) + .map((comment) => comment.createdAt)); +} + +function isTrustedMarkerComment(comment, marker, trustedAuthors) { + if (typeof comment?.body !== 'string' || !comment.body.includes(marker)) return false; + if (!trustedAuthors) return true; + return trustedAuthors.has(normalizeLogin(comment.authorLogin || comment.author?.login)); } function latestLabelEventAt(events, label, type) { diff --git a/tests/github-sheriff.test.mjs b/tests/github-sheriff.test.mjs index bd9107285..d45a57227 100644 --- a/tests/github-sheriff.test.mjs +++ b/tests/github-sheriff.test.mjs @@ -5,29 +5,33 @@ import { CLOSE_MARKER, WARNING_MARKER, evaluatePullRequest, + mergeIssueComments, mergeIssueLabelEvents, + normalizePullRequest, parseArgs, } from '../scripts/github/sheriff.mjs'; const NOW = '2026-07-08T00:00:00Z'; describe('github sheriff', () => { - it('warns a contributor-blocked PR after one week open', () => { + it('warns a contributor-blocked PR after one week waiting on contributor action', () => { const plan = evaluatePullRequest(pr({ createdAt: '2026-06-30T00:00:00Z', latestCommitAt: '2026-06-30T01:00:00Z', comments: [ - comment('pbakaus', '2026-07-03T00:00:00Z', '/sheriff wait'), + comment('pbakaus', '2026-07-01T00:00:00Z', '/sheriff wait'), ], }), { now: NOW }); assert.equal(plan.contributorActionRequired, true); assert.equal(plan.daysOpen, 8); + assert.equal(plan.waitingDays, 7); assert.deepEqual(plan.labelsToAdd, ['stale', 'waiting on contributor']); assert.equal(plan.shouldWarn, true); assert.equal(plan.shouldClose, false); assert.match(plan.warningComment, /waiting on contributor action/); - assert.match(plan.warningComment, /2026-07-14/); + assert.match(plan.warningComment, /waiting for contributor action for 7 days/); + assert.match(plan.warningComment, /2026-07-15/); }); it('closes a non-regular contributor PR that is still waiting after two weeks open', () => { @@ -64,7 +68,7 @@ describe('github sheriff', () => { assert.doesNotMatch(plan.warningComment, /2026-07-04/); }); - it('uses the stale label as durable proof that a warning already happened', () => { + it('does not treat a stale label without a current warning marker as proof of warning', () => { const plan = evaluatePullRequest(pr({ createdAt: '2026-06-20T00:00:00Z', latestCommitAt: '2026-06-20T01:00:00Z', @@ -77,10 +81,66 @@ describe('github sheriff', () => { ], }), { now: NOW }); + assert.equal(plan.shouldWarn, true); + assert.equal(plan.shouldClose, false); + }); + + it('ignores stale warning markers from untrusted commenters', () => { + const plan = evaluatePullRequest(pr({ + createdAt: '2026-06-20T00:00:00Z', + latestCommitAt: '2026-06-20T01:00:00Z', + comments: [ + comment('pbakaus', '2026-06-21T00:00:00Z', '/sheriff wait'), + comment('drive-by', '2026-06-27T00:00:00Z', WARNING_MARKER), + ], + }), { now: NOW }); + + assert.equal(plan.shouldWarn, true); + assert.equal(plan.shouldClose, false); + }); + + it('ignores close markers from untrusted commenters', () => { + const plan = evaluatePullRequest(pr({ + createdAt: '2026-06-20T00:00:00Z', + latestCommitAt: '2026-06-20T01:00:00Z', + comments: [ + comment('pbakaus', '2026-06-21T00:00:00Z', '/sheriff wait'), + comment('github-actions[bot]', '2026-06-27T00:00:00Z', WARNING_MARKER), + comment('drive-by', '2026-07-04T00:00:00Z', CLOSE_MARKER), + ], + }), { now: NOW }); + assert.equal(plan.shouldWarn, false); assert.equal(plan.shouldClose, true); }); + it('keeps old trusted warning markers after full issue comment hydration', () => { + const prUnderReview = pr({ + createdAt: '2026-06-20T00:00:00Z', + latestCommitAt: '2026-06-20T01:00:00Z', + labels: ['waiting on contributor', 'stale'], + labelEvents: [ + labelEvent('LabeledEvent', 'waiting on contributor', 'pbakaus', '2026-06-21T00:00:00Z'), + ], + comments: Array.from({ length: 50 }, (_, index) => ( + comment('review-bot', `2026-06-28T00:${String(index).padStart(2, '0')}:00Z`, 'follow-up') + )), + }); + + const truncatedPlan = evaluatePullRequest(prUnderReview, { now: NOW }); + mergeIssueComments(prUnderReview, [ + restComment('pbakaus', '2026-06-21T00:00:00Z', 'Please fix the review feedback.'), + restComment('github-actions[bot]', '2026-06-27T00:00:00Z', WARNING_MARKER), + ...prUnderReview.comments, + ]); + const hydratedPlan = evaluatePullRequest(prUnderReview, { now: NOW }); + + assert.equal(truncatedPlan.shouldWarn, true); + assert.equal(truncatedPlan.shouldClose, false); + assert.equal(hydratedPlan.shouldWarn, false); + assert.equal(hydratedPlan.shouldClose, true); + }); + it('does not infer contributor blockers from maintainer prose', () => { for (const body of [ 'LGTM, merging after CI.', @@ -320,6 +380,103 @@ describe('github sheriff', () => { assert.equal(plan.shouldClose, false); }); + it('uses PR creation, not unrelated updates, for drafts opened as draft', () => { + const plan = evaluatePullRequest(pr({ + isDraft: true, + createdAt: '2026-06-20T00:00:00Z', + updatedAt: '2026-07-07T00:00:00Z', + latestCommitAt: '2026-06-20T01:00:00Z', + }), { now: NOW }); + + assert.equal(plan.contributorActionRequired, true); + assert.equal(plan.waitingDays, 18); + assert.deepEqual(plan.labelsToAdd, ['stale', 'waiting on contributor']); + assert.equal(plan.shouldWarn, true); + }); + + it('uses the latest convert-to-draft event for PRs converted back to draft', () => { + const plan = evaluatePullRequest(pr({ + isDraft: true, + createdAt: '2026-06-20T00:00:00Z', + updatedAt: '2026-07-07T00:00:00Z', + latestCommitAt: '2026-06-20T01:00:00Z', + draftEvents: [ + draftEvent('ReadyForReviewEvent', '2026-06-22T00:00:00Z'), + draftEvent('ConvertToDraftEvent', '2026-07-06T00:00:00Z'), + ], + }), { now: NOW }); + + assert.equal(plan.contributorActionRequired, true); + assert.equal(plan.waitingDays, 2); + assert.deepEqual(plan.labelsToAdd, ['waiting on contributor']); + assert.equal(plan.shouldWarn, false); + }); + + it('uses the latest ready-for-review event as a lower bound when the current draft conversion is missing', () => { + const plan = evaluatePullRequest(pr({ + isDraft: true, + createdAt: '2026-06-20T00:00:00Z', + updatedAt: '2026-07-07T00:00:00Z', + latestCommitAt: '2026-06-20T01:00:00Z', + draftEvents: [ + draftEvent('ConvertToDraftEvent', '2026-06-22T00:00:00Z'), + draftEvent('ReadyForReviewEvent', '2026-07-06T00:00:00Z'), + ], + }), { now: NOW }); + + assert.equal(plan.contributorActionRequired, true); + assert.equal(plan.waitingDays, 2); + assert.deepEqual(plan.labelsToAdd, ['waiting on contributor']); + assert.equal(plan.shouldWarn, false); + }); + + it('normalizes draft transitions from a dedicated timeline slice', () => { + const normalized = normalizePullRequest(graphqlPrNode({ + isDraft: true, + createdAt: '2026-06-20T00:00:00Z', + updatedAt: '2026-07-07T00:00:00Z', + labelTimelineItems: { + nodes: [ + { + __typename: 'LabeledEvent', + label: { name: 'waiting on contributor' }, + actor: { login: 'pbakaus' }, + createdAt: '2026-07-06T00:00:00Z', + }, + ], + }, + draftTimelineItems: { + nodes: [ + { + __typename: 'ConvertToDraftEvent', + actor: { login: 'contrib' }, + createdAt: '2026-07-06T00:00:00Z', + }, + ], + }, + })); + + const plan = evaluatePullRequest(normalized, { now: NOW }); + + assert.deepEqual(normalized.labelEvents, [ + { + type: 'LabeledEvent', + label: 'waiting on contributor', + actorLogin: 'pbakaus', + createdAt: '2026-07-06T00:00:00Z', + }, + ]); + assert.deepEqual(normalized.draftEvents, [ + { + type: 'ConvertToDraftEvent', + actorLogin: 'contrib', + createdAt: '2026-07-06T00:00:00Z', + }, + ]); + assert.equal(plan.waitingDays, 2); + assert.equal(plan.shouldWarn, false); + }); + it('marks passing resolved PRs as ready to merge', () => { const plan = evaluatePullRequest(pr({ createdAt: '2026-07-01T00:00:00Z', @@ -370,6 +527,56 @@ describe('github sheriff', () => { assert.deepEqual(plan.labelsToAdd, ['ready to merge']); }); + it('does not warn immediately when an old PR receives fresh requested changes', () => { + const plan = evaluatePullRequest(pr({ + createdAt: '2026-06-20T00:00:00Z', + latestCommitAt: '2026-06-21T00:00:00Z', + statusState: 'SUCCESS', + mergeable: 'MERGEABLE', + reviewDecision: 'CHANGES_REQUESTED', + reviews: [ + { + authorLogin: 'pbakaus', + state: 'CHANGES_REQUESTED', + submittedAt: '2026-07-07T12:00:00Z', + body: 'Needs one follow-up.', + }, + ], + }), { now: NOW }); + + assert.equal(plan.daysOpen, 18); + assert.equal(plan.waitingDays, 0); + assert.equal(plan.contributorActionRequired, true); + assert.deepEqual(plan.labelsToAdd, ['blocked: review threads', 'waiting on contributor']); + assert.equal(plan.shouldWarn, false); + assert.equal(plan.shouldClose, false); + }); + + it('requires a warning after the current blocker before closing', () => { + const plan = evaluatePullRequest(pr({ + createdAt: '2026-06-20T00:00:00Z', + latestCommitAt: '2026-06-21T00:00:00Z', + statusState: 'SUCCESS', + mergeable: 'MERGEABLE', + reviewDecision: 'CHANGES_REQUESTED', + comments: [ + comment('github-actions[bot]', '2026-06-30T00:00:00Z', WARNING_MARKER), + ], + reviews: [ + { + authorLogin: 'pbakaus', + state: 'CHANGES_REQUESTED', + submittedAt: '2026-07-07T00:00:00Z', + body: 'Needs one follow-up.', + }, + ], + }), { now: '2026-07-15T00:00:00Z' }); + + assert.equal(plan.waitingDays, 8); + assert.equal(plan.shouldWarn, true); + assert.equal(plan.shouldClose, false); + }); + it('blocks current changes-requested reviews until the contributor responds', () => { const plan = evaluatePullRequest(pr({ createdAt: '2026-07-04T00:00:00Z', @@ -471,6 +678,7 @@ function pr(overrides = {}) { reviews: [], reviewThreads: [], labelEvents: [], + draftEvents: [], latestCommitAt: '2026-07-01T01:00:00Z', latestCommitAuthorLogin: 'contrib', latestCommitCommitterLogin: 'contrib', @@ -484,6 +692,10 @@ function comment(authorLogin, createdAt, body) { return { authorLogin, createdAt, body }; } +function restComment(login, createdAt, body) { + return { user: { login }, created_at: createdAt, body }; +} + function labelEvent(type, label, actorLogin, createdAt) { return { type, label, actorLogin, createdAt }; } @@ -496,3 +708,29 @@ function issueEvent(event, label, actorLogin, createdAt) { created_at: createdAt, }; } + +function draftEvent(type, createdAt, actorLogin = 'contrib') { + return { type, createdAt, actorLogin }; +} + +function graphqlPrNode(overrides = {}) { + return { + number: 123, + title: 'Test PR', + url: 'https://github.com/pbakaus/impeccable/pull/123', + isDraft: false, + createdAt: '2026-07-01T00:00:00Z', + updatedAt: '2026-07-01T00:00:00Z', + mergeable: 'MERGEABLE', + reviewDecision: null, + author: { login: 'contrib' }, + labels: { nodes: [] }, + comments: { nodes: [] }, + reviews: { nodes: [] }, + commits: { nodes: [] }, + reviewThreads: { nodes: [] }, + labelTimelineItems: { nodes: [] }, + draftTimelineItems: { nodes: [] }, + ...overrides, + }; +}