From a8ad2cf69928991e89545e93b8577e9f04fc03fb Mon Sep 17 00:00:00 2001 From: Paul Bakaus Date: Thu, 9 Jul 2026 10:04:09 -0700 Subject: [PATCH] Soften sheriff stale classification --- scripts/github/sheriff.mjs | 34 ++++++++++++++++++++++++---------- tests/github-sheriff.test.mjs | 28 +++++++++++++++++++++++++--- 2 files changed, 49 insertions(+), 13 deletions(-) diff --git a/scripts/github/sheriff.mjs b/scripts/github/sheriff.mjs index eed89c843..e4539eefd 100644 --- a/scripts/github/sheriff.mjs +++ b/scripts/github/sheriff.mjs @@ -145,24 +145,26 @@ export function evaluatePullRequest(pr, options = {}) { const unresolvedThreadBlockers = unresolvedThreadsNeedingContributor(pr.reviewThreads || [], author); const blockers = []; + const addBlocker = (blocker) => blockers.push({ contributorAction: false, ...blocker }); + const addContributorBlocker = (blocker) => blockers.push({ contributorAction: true, ...blocker }); if (pr.isDraft) { - blockers.push({ kind: 'draft' }); + addContributorBlocker({ kind: 'draft' }); } if (FAILING_STATUS_STATES.has(pr.statusState)) { - blockers.push({ kind: 'ci', label: 'blocked: ci', at: pr.latestCommitAt || pr.updatedAt }); + addBlocker({ kind: 'ci', label: 'blocked: ci', at: pr.latestCommitAt || pr.updatedAt }); } if (pr.mergeable === 'CONFLICTING') { - blockers.push({ kind: 'merge-conflicts', label: 'blocked: merge conflicts', at: pr.updatedAt }); + addBlocker({ kind: 'merge-conflicts', label: 'blocked: merge conflicts', at: pr.updatedAt }); } if (latestBlockingReviewAt && !isAfter(latestContributorAt, latestBlockingReviewAt)) { - blockers.push({ kind: 'changes-requested', label: 'blocked: review threads', at: latestBlockingReviewAt }); + addContributorBlocker({ kind: 'changes-requested', label: 'blocked: review threads', at: latestBlockingReviewAt }); } if (unresolvedThreadBlockers.length > 0) { - blockers.push({ + addContributorBlocker({ kind: 'review-threads', label: 'blocked: review threads', at: latestDate(unresolvedThreadBlockers.map((thread) => thread.latestCommentAt)), @@ -170,15 +172,20 @@ export function evaluatePullRequest(pr, options = {}) { } if (latestMaintainerWaitAt && !isAfter(latestContributorAt, latestMaintainerWaitAt)) { - blockers.push({ kind: 'sheriff-wait', at: latestMaintainerWaitAt }); + addContributorBlocker({ kind: 'sheriff-wait', at: latestMaintainerWaitAt }); } - const waitingLabelAt = latestLabelEventAt(pr.labelEvents || [], 'waiting on contributor', 'LabeledEvent'); + const waitingLabelAt = latestMaintainerLabelEventAt( + pr.labelEvents || [], + 'waiting on contributor', + 'LabeledEvent', + maintainers, + ); if (labels.has('waiting on contributor') && waitingLabelAt && !isAfter(latestContributorAt, waitingLabelAt)) { - blockers.push({ kind: 'manual-waiting', at: waitingLabelAt }); + addContributorBlocker({ kind: 'manual-waiting', at: waitingLabelAt }); } - const contributorActionRequired = blockers.length > 0; + const contributorActionRequired = blockers.some((blocker) => blocker.contributorAction); const unresolvedThreadCount = (pr.reviewThreads || []).filter((thread) => !thread.isResolved).length; const statusIsReady = pr.statusState === 'SUCCESS'; const mergeableIsReady = pr.mergeable === 'MERGEABLE'; @@ -320,7 +327,7 @@ 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, CI failure, merge conflict, 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 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.`, '', `If nothing changes, this PR may be closed on or after ${closeDate}. Happy to reopen when it is ready to continue.`, ].join('\n'); @@ -615,6 +622,13 @@ function latestLabelEventAt(events, label, type) { .map((event) => event.createdAt)); } +function latestMaintainerLabelEventAt(events, label, type, maintainers) { + return latestDate(events + .filter((event) => event.type === type && event.label === label) + .filter((event) => maintainers.has(normalizeLogin(event.actorLogin))) + .map((event) => event.createdAt)); +} + function issueEventType(event) { if (event === 'labeled') return 'LabeledEvent'; if (event === 'unlabeled') return 'UnlabeledEvent'; diff --git a/tests/github-sheriff.test.mjs b/tests/github-sheriff.test.mjs index 2dba43445..bd9107285 100644 --- a/tests/github-sheriff.test.mjs +++ b/tests/github-sheriff.test.mjs @@ -288,14 +288,36 @@ describe('github sheriff', () => { assert.equal(plan.shouldClose, false); }); - it('marks failing checks as waiting on contributor', () => { + it('marks failing checks as blocked without starting the contributor stale clock', () => { const plan = evaluatePullRequest(pr({ createdAt: '2026-07-01T00:00:00Z', statusState: 'FAILURE', }), { now: NOW }); - assert.equal(plan.contributorActionRequired, true); - assert.ok(plan.desiredLabels.includes('blocked: ci')); + assert.equal(plan.contributorActionRequired, false); + assert.deepEqual(plan.labelsToAdd, ['blocked: ci', 'needs maintainer review']); + assert.equal(plan.shouldWarn, false); + assert.equal(plan.shouldClose, false); + }); + + it('does not warn or close PRs blocked only by merge conflicts', () => { + const plan = evaluatePullRequest(pr({ + createdAt: '2026-06-20T00:00:00Z', + mergeable: 'CONFLICTING', + labels: ['waiting on contributor', 'stale'], + labelEvents: [ + labelEvent('LabeledEvent', 'waiting on contributor', 'github-actions[bot]', '2026-07-01T00:00:00Z'), + ], + comments: [ + comment('github-actions[bot]', '2026-07-01T00:00:00Z', WARNING_MARKER), + ], + }), { now: NOW }); + + assert.equal(plan.contributorActionRequired, false); + assert.deepEqual(plan.labelsToAdd, ['blocked: merge conflicts', 'needs maintainer review']); + assert.deepEqual(plan.labelsToRemove, ['stale', 'waiting on contributor']); + assert.equal(plan.shouldWarn, false); + assert.equal(plan.shouldClose, false); }); it('marks passing resolved PRs as ready to merge', () => {