From fa5fa255291b6715954ef9f6e5abb09eaed720c5 Mon Sep 17 00:00:00 2001 From: copilot <223556219+Copilot@users.noreply.github.com> Date: Mon, 21 Sep 2026 21:40:25 +0000 Subject: [PATCH] fix: clear stale pr-queue-hygiene conflict marker and paginate comments Fixes two defects in scripts/pr-queue-hygiene.mjs from the review of #240: - On the non-conflicting path, the loop previously continued without clearing the first-conflict marker or the needs-rebase-or-close label. If the PR later reconflicted, hoursSince() was computed from the stale marker's timestamp, so the PR was flagged as stale immediately instead of getting a fresh 48-hour window. A trusted 'resolved' marker is now posted when a PR stops conflicting, and findFirstConflictObservedAt() ignores any first-conflict marker predating the latest trusted resolved marker. - The comment lookup only read a single per_page=100 page. Since issue comments sort ascending, a PR with more than 100 comments could have its marker on an earlier page that was never fetched, causing a duplicate marker to be posted every run and the 48h gate to never fire. fetchAllIssueComments() now paginates through every page, the same way fetchAllOpenPRs() already does for PRs. Fixes cncf/endusers#432 Signed-off-by: copilot <223556219+Copilot@users.noreply.github.com> --- scripts/pr-queue-hygiene.mjs | 114 ++++++++++++++++++++++++++++---- tests/pr-queue-hygiene.test.mjs | 51 ++++++++++++++ 2 files changed, 151 insertions(+), 14 deletions(-) diff --git a/scripts/pr-queue-hygiene.mjs b/scripts/pr-queue-hygiene.mjs index 0f26345b..1fec1f14 100755 --- a/scripts/pr-queue-hygiene.mjs +++ b/scripts/pr-queue-hygiene.mjs @@ -28,6 +28,14 @@ // Otherwise any commenter could post a far-future marker to exempt a PR // from the 48h gate forever, or a backdated one to have this job label // and nag somebody else's freshly-conflicting PR. +// 5. Marker clearing: when a PR stops conflicting, its first-conflict +// marker is neutralized with a trusted "resolved" marker (and the +// `needs-rebase-or-close` label is removed), so a later reconflict +// starts a fresh 48h window instead of inheriting the old marker's +// timestamp and being flagged as stale immediately. +// 6. Comment pagination: issue comments are fetched a page at a time +// instead of relying on a single `per_page=100` call, so a marker +// posted early in a PR with a long comment thread is still found. // // Usage: node scripts/pr-queue-hygiene.mjs // Requires `gh` authenticated with `pull-requests: write` on this repo (as @@ -46,6 +54,13 @@ export const HOLD_LABELS = new Set(['hold', 'on-hold', 'do-not-merge']); export const MARKER_PREFIX = '/; +// Posted once a previously-conflicting PR is observed as no longer +// conflicting, so a later re-conflict starts a fresh 48h window instead of +// inheriting the stale first-conflict marker's timestamp (see +// findFirstConflictObservedAt). +export const RESOLVED_MARKER_PREFIX = '/; + // Logins whose markers are believed. The marker is this job's state store, // but it is kept in a comment thread every GitHub user can write to, so an // unauthenticated marker is an unauthenticated state write. @@ -68,26 +83,53 @@ export function isTrustedMarkerComment( return trustedLogins.has(String(user.login || '').toLowerCase()); } +// A resolved marker is trusted under the same rule as a first-conflict +// marker: only a bot login on the allowlist can clear the state, otherwise +// anyone could post a fake resolution to reset another PR's 48h clock. +export function isTrustedResolvedComment( + comment, + trustedLogins = TRUSTED_MARKER_AUTHORS, +) { + return isTrustedMarkerComment(comment, trustedLogins); +} + // Extracts the first-conflict-observed timestamp (if any) from a PR's // issue comments, by finding this script's own hidden marker comment. // -// Markers from anyone else are ignored rather than returned, and so are -// markers whose payload is not a parseable past timestamp: a future value -// would make hoursSince() negative and exempt the PR from the stale gate -// permanently. Scanning continues past a rejected marker so a spoofed -// comment cannot hide the genuine one behind it. When nothing is trusted the -// caller records a fresh marker, so the state self-heals. +// Comments are scanned in ascending (oldest-first) order. A trusted +// "resolved" marker clears any first-conflict marker seen so far, so a PR +// that stopped conflicting and later reconflicts gets a fresh candidate +// timestamp from the marker posted *after* the resolution, rather than +// reporting the stale pre-resolution marker and being immediately flagged +// as >48h stale. Markers from anyone else are ignored rather than +// returned, and so are markers whose payload is not a parseable past +// timestamp: a future value would make hoursSince() negative and exempt +// the PR from the stale gate permanently. Scanning continues past a +// rejected marker so a spoofed comment cannot hide the genuine one behind +// it. When nothing is trusted the caller records a fresh marker, so the +// state self-heals. export function findFirstConflictObservedAt(comments, options = {}) { - const { isTrusted = isTrustedMarkerComment, now = Date.now() } = options; + const { + isTrusted = isTrustedMarkerComment, + isResolvedTrusted = isTrustedResolvedComment, + now = Date.now(), + } = options; + let candidate = null; for (const comment of comments) { + const resolvedMatch = RESOLVED_MARKER_RE.exec(comment.body || ''); + if (resolvedMatch && isResolvedTrusted(comment)) { + candidate = null; + continue; + } + const match = MARKER_RE.exec(comment.body || ''); if (!match) continue; if (!isTrusted(comment)) continue; const parsed = Date.parse(match[1]); if (Number.isNaN(parsed) || parsed > now) continue; - return match[1]; + candidate = match[1]; } - return null; + return candidate; } function gh(args) { @@ -113,6 +155,27 @@ export function fetchAllOpenPRs(repo) { return prs; } +// Fetches every issue comment on a PR, paginating past the 100-per-page +// cap. Issue comments sort ascending, so on a PR with more than 100 +// comments a single unpaginated page can miss the marker entirely if it +// was posted early on -- this walks every page instead. +export function fetchAllIssueComments(repo, number) { + const comments = []; + let page = 1; + const perPage = 100; + for (;;) { + const raw = gh([ + 'api', + `repos/${repo}/issues/${number}/comments?per_page=${perPage}&page=${page}`, + ]); + const batch = JSON.parse(raw); + comments.push(...batch); + if (batch.length < perPage) break; + page += 1; + } + return comments; +} + export function hoursSince(isoString, now = Date.now()) { return (now - Date.parse(isoString)) / (1000 * 60 * 60); } @@ -176,14 +239,37 @@ async function main() { const conflicting = await isConflicting(REPO, number); - const commentsRaw = gh([ - 'api', - `repos/${REPO}/issues/${number}/comments?per_page=100`, - ]); - const comments = JSON.parse(commentsRaw); + const comments = fetchAllIssueComments(REPO, number); const firstObservedAt = findFirstConflictObservedAt(comments); if (!conflicting) { + if (firstObservedAt) { + console.log( + `PR #${number}: no longer conflicting, clearing stale marker`, + ); + if (!DRY_RUN) { + if (labelNames(pr).includes(LABEL)) { + gh([ + 'pr', + 'edit', + String(number), + '--repo', + REPO, + '--remove-label', + LABEL, + ]); + } + gh([ + 'pr', + 'comment', + String(number), + '--repo', + REPO, + '--body', + `${RESOLVED_MARKER_PREFIX}${new Date().toISOString()} -->`, + ]); + } + } continue; } diff --git a/tests/pr-queue-hygiene.test.mjs b/tests/pr-queue-hygiene.test.mjs index 6ddf374b..d06f5804 100644 --- a/tests/pr-queue-hygiene.test.mjs +++ b/tests/pr-queue-hygiene.test.mjs @@ -6,7 +6,9 @@ import { isConflicting, isHeld, isTrustedMarkerComment, + isTrustedResolvedComment, MARKER_PREFIX, + RESOLVED_MARKER_PREFIX, } from '../scripts/pr-queue-hygiene.mjs'; const BOT = { login: 'github-actions[bot]', type: 'Bot' }; @@ -16,6 +18,10 @@ function marker(timestamp, user = BOT) { return { body: `${MARKER_PREFIX}${timestamp} -->`, user }; } +function resolvedMarker(timestamp, user = BOT) { + return { body: `${RESOLVED_MARKER_PREFIX}${timestamp} -->`, user }; +} + test('findFirstConflictObservedAt: finds the marker comment among others', () => { const comments = [ { body: 'unrelated comment', user: HUMAN }, @@ -77,6 +83,51 @@ test('findFirstConflictObservedAt: tolerates a comment with no author', () => { ); }); +test('findFirstConflictObservedAt: a trusted resolved marker clears an earlier stale marker', () => { + const comments = [ + marker('2020-01-01T00:00:00.000Z'), + resolvedMarker('2020-01-02T00:00:00.000Z'), + ]; + assert.equal( + findFirstConflictObservedAt(comments, { + now: Date.parse('2026-09-03T00:00:00.000Z'), + }), + null, + ); +}); + +test('findFirstConflictObservedAt: a fresh marker after resolution is used, not the pre-resolution one', () => { + const comments = [ + marker('2020-01-01T00:00:00.000Z'), + resolvedMarker('2020-01-02T00:00:00.000Z'), + marker('2026-09-01T00:00:00.000Z'), + ]; + assert.equal( + findFirstConflictObservedAt(comments, { + now: Date.parse('2026-09-03T00:00:00.000Z'), + }), + '2026-09-01T00:00:00.000Z', + ); +}); + +test('findFirstConflictObservedAt: an untrusted resolved marker does not clear the genuine one', () => { + const comments = [ + marker('2020-01-01T00:00:00.000Z'), + resolvedMarker('2020-01-02T00:00:00.000Z', HUMAN), + ]; + assert.equal( + findFirstConflictObservedAt(comments, { + now: Date.parse('2026-09-03T00:00:00.000Z'), + }), + '2020-01-01T00:00:00.000Z', + ); +}); + +test('isTrustedResolvedComment: only an allowlisted bot login is trusted', () => { + assert.equal(isTrustedResolvedComment({ user: BOT }), true); + assert.equal(isTrustedResolvedComment({ user: HUMAN }), false); +}); + test('isTrustedMarkerComment: only an allowlisted bot login is trusted', () => { assert.equal(isTrustedMarkerComment({ user: BOT }), true); assert.equal(isTrustedMarkerComment({ user: HUMAN }), false);