Base sheriff stale clock on blocker age (#364)

This commit is contained in:
Paul Bakaus
2026-07-10 12:02:52 -07:00
committed by GitHub
parent da99645a58
commit 630fc2682a
2 changed files with 342 additions and 22 deletions
+100 -18
View File
@@ -38,6 +38,7 @@ export const CLOSE_MARKER = '<!-- impeccable-sheriff:auto-close -->';
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) {
+242 -4
View File
@@ -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,
};
}