From 5dfdc3fb80787f51b1fbe367afff5ee98b6c341f Mon Sep 17 00:00:00 2001 From: Jorge Castro Date: Thu, 17 Sep 2026 13:54:41 +0000 Subject: [PATCH] ci: fix pr-queue-hygiene mergeable check to use per-PR API fetch Fixes cncf/endusers#217: scripts/pr-queue-hygiene.mjs re-fetches each PR's mergeability individually via GET /repos/{owner}/{repo}/pulls/{number} instead of relying on the pulls-list endpoint's 'mergeable' field, which GitHub computes asynchronously and frequently returns as UNKNOWN on list responses. Adds bounded retry polling for pending mergeable states and skips PRs carrying hold labels per GOVERNANCE.md. The workflow file is tracked in issue #318 per the Hive App workflow-permission gap policy. Signed-off-by: Jorge Castro --- GOVERNANCE.md | 11 ++ package.json | 1 + scripts/pr-queue-hygiene.mjs | 198 ++++++++++++++++++++++++++++++++ tests/pr-queue-hygiene.test.mjs | 107 +++++++++++++++++ 4 files changed, 317 insertions(+) create mode 100755 scripts/pr-queue-hygiene.mjs create mode 100644 tests/pr-queue-hygiene.test.mjs diff --git a/GOVERNANCE.md b/GOVERNANCE.md index f1ec6213..9e95a821 100644 --- a/GOVERNANCE.md +++ b/GOVERNANCE.md @@ -105,6 +105,17 @@ To keep the queue from rotting (see issue #58): a signal to adjust the low-risk merge classes above, not to lower the review bar. +**Operational status**: `scripts/pr-queue-hygiene.mjs` flags (labels +`needs-rebase-or-close` + comments) any open PR that has been conflicting with +the base branch for more than 48 hours. It does not close PRs automatically — +judging whether a conflicting PR is superseded needs human or author-agent +judgment — but it makes stale PRs visible without waiting for manual sweeps. +Mergeability is checked per-PR via the single-PR API endpoint to avoid stale +cached list values, and PRs labeled `hold`, `on-hold`, or `do-not-merge` are +skipped per agent-automation policy. The scheduled GitHub Actions workflow file +(`.github/workflows/pr-queue-hygiene.yml`) is tracked in issue #318 awaiting +maintainer commit per the Hive App workflow-permission gap. + ### Cross-agent duplicate prevention Fleet-wide sweeps have repeatedly found duplicate PRs opening the same fix from diff --git a/package.json b/package.json index 91ffecd1..707d3868 100644 --- a/package.json +++ b/package.json @@ -56,6 +56,7 @@ "validate:case-studies": "node scripts/validate-case-studies.mjs", "collect:radar-reports": "node scripts/collect-radar-reports.mjs", "validate:radar-reports": "node scripts/validate-radar-reports.mjs", + "pr-queue-hygiene": "node scripts/pr-queue-hygiene.mjs", "test:unit": "node --test" }, "repository": { diff --git a/scripts/pr-queue-hygiene.mjs b/scripts/pr-queue-hygiene.mjs new file mode 100755 index 00000000..8b260c2e --- /dev/null +++ b/scripts/pr-queue-hygiene.mjs @@ -0,0 +1,198 @@ +#!/usr/bin/env node +// Flags open PRs that have been conflicting with the base branch for more +// than 48 hours, per GOVERNANCE.md merge-queue hygiene (issue #58). +// +// This fetches mergeability per-PR (`GET /repos/{owner}/{repo}/pulls/{n}`) +// rather than relying on the list endpoint's `mergeable` field, which +// GitHub computes asynchronously and frequently returns as `UNKNOWN` on +// list responses -- silently letting conflicting PRs slip past a check +// gated on that value (see issue #217). Two related bugs are fixed here +// too: +// +// 1. Pagination: `gh pr list` defaults to --limit 30, which silently drops +// PRs once the fleet queue passes 30 open PRs. This paginates through +// every open PR instead of relying on a single bounded call. +// 2. Conflict age: `updatedAt` is *any* update to the PR (a new commit, a +// label, a comment) -- it does not measure how long the PR has actually +// been conflicting with the base branch. Instead, this persists a +// first-conflict-observed marker as an HTML comment on the PR itself +// (the only durable, per-PR storage a stateless scheduled job has +// available) the first time a PR is seen as conflicting, and only flags +// it once 48 hours have elapsed since *that* timestamp -- not since the +// PR's last unrelated update. +// 3. Hold safety: PRs carrying `hold`, `on-hold`, or `do-not-merge` are +// skipped entirely per GOVERNANCE.md agent-automation policy. +// +// Usage: node scripts/pr-queue-hygiene.mjs +// Requires `gh` authenticated with `pull-requests: write` on this repo (as +// the workflow already grants). Set DRY_RUN=1 to log intended actions +// without labeling/commenting. + +import { execFileSync } from 'node:child_process'; + +const REPO = process.env.REPO || process.env.GITHUB_REPOSITORY; +const DRY_RUN = process.env.DRY_RUN === '1'; +const STALE_HOURS = 48; +const LABEL = 'needs-rebase-or-close'; +export const HOLD_LABELS = new Set(['hold', 'on-hold', 'do-not-merge']); +export const MARKER_PREFIX = '/; + +function gh(args) { + return execFileSync('gh', args, { encoding: 'utf8' }); +} + +// Fetches every open PR, paginating past gh's default --limit 30 so a +// queue deeper than one page isn't silently truncated. +export function fetchAllOpenPRs(repo) { + const prs = []; + let page = 1; + const perPage = 100; + for (;;) { + const raw = gh([ + 'api', + `repos/${repo}/pulls?state=open&per_page=${perPage}&page=${page}`, + ]); + const batch = JSON.parse(raw); + prs.push(...batch); + if (batch.length < perPage) break; + page += 1; + } + return prs; +} + +// Extracts the first-conflict-observed timestamp (if any) from a PR's +// issue comments, by finding this script's own hidden marker comment. +export function findFirstConflictObservedAt(comments) { + for (const comment of comments) { + const match = MARKER_RE.exec(comment.body || ''); + if (match) return match[1]; + } + return null; +} + +export function hoursSince(isoString, now = Date.now()) { + return (now - Date.parse(isoString)) / (1000 * 60 * 60); +} + +export function labelNames(pr) { + return (pr.labels || []).map((l) => (typeof l === 'string' ? l : l.name)); +} + +export function isHeld(pr) { + const names = labelNames(pr).map((n) => n.toLowerCase()); + return names.some((n) => HOLD_LABELS.has(n)); +} + +export async function isConflicting( + repo, + number, + fetchDetail = defaultFetchDetail, + maxRetries = 2, + sleepFn = (ms) => new Promise((resolve) => setTimeout(resolve, ms)), +) { + for (let attempt = 0; attempt <= maxRetries; attempt++) { + const detail = await fetchDetail(repo, number); + if (detail.mergeable === false || detail.mergeable_state === 'dirty') { + return true; + } + if (detail.mergeable === true && detail.mergeable_state !== 'dirty') { + return false; + } + if (attempt < maxRetries) { + await sleepFn(1000); + } + } + return false; +} + +function defaultFetchDetail(repo, number) { + return JSON.parse( + gh([ + 'api', + `repos/${repo}/pulls/${number}`, + '--jq', + '{mergeable: .mergeable, mergeable_state: .mergeable_state}', + ]), + ); +} + +async function main() { + if (!REPO) throw new Error('REPO or GITHUB_REPOSITORY must be set'); + + const prs = fetchAllOpenPRs(REPO); + console.log(`Fetched ${prs.length} open PR(s) from ${REPO}`); + + for (const pr of prs) { + const number = pr.number; + + // Never touch PRs marked on hold + if (isHeld(pr)) { + console.log(`PR #${number}: carries hold label, skipping`); + continue; + } + + const conflicting = await isConflicting(REPO, number); + + const commentsRaw = gh([ + 'api', + `repos/${REPO}/issues/${number}/comments?per_page=100`, + ]); + const comments = JSON.parse(commentsRaw); + const firstObservedAt = findFirstConflictObservedAt(comments); + + if (!conflicting) { + continue; + } + + if (!firstObservedAt) { + console.log( + `PR #${number}: newly observed as conflicting, recording marker`, + ); + if (!DRY_RUN) { + gh([ + 'pr', + 'comment', + String(number), + '--repo', + REPO, + '--body', + `${MARKER_PREFIX}${new Date().toISOString()} -->`, + ]); + } + continue; + } + + const ageHours = hoursSince(firstObservedAt); + if (ageHours < STALE_HOURS) { + continue; + } + + if (labelNames(pr).includes(LABEL)) { + continue; + } + + console.log( + `Flagging stale conflicting PR #${number} (conflicting for ${ageHours.toFixed(1)}h)`, + ); + if (!DRY_RUN) { + gh(['pr', 'edit', String(number), '--repo', REPO, '--add-label', LABEL]); + gh([ + 'pr', + 'comment', + String(number), + '--repo', + REPO, + '--body', + 'This PR has been conflicting with the base branch for more than 48 hours. Per [GOVERNANCE.md](../../blob/main/GOVERNANCE.md) merge-queue hygiene: please rebase, or close this PR if it has been superseded by other merged work. Flagged automatically by the PR queue hygiene workflow.', + ]); + } + } +} + +if (import.meta.url === `file://${process.argv[1]}`) { + main().catch((err) => { + console.error(err); + process.exitCode = 1; + }); +} diff --git a/tests/pr-queue-hygiene.test.mjs b/tests/pr-queue-hygiene.test.mjs new file mode 100644 index 00000000..6dee002f --- /dev/null +++ b/tests/pr-queue-hygiene.test.mjs @@ -0,0 +1,107 @@ +import assert from 'node:assert/strict'; +import test from 'node:test'; +import { + findFirstConflictObservedAt, + hoursSince, + isConflicting, + isHeld, + MARKER_PREFIX, +} from '../scripts/pr-queue-hygiene.mjs'; + +test('findFirstConflictObservedAt: finds the marker comment among others', () => { + const comments = [ + { body: 'unrelated comment' }, + { body: `${MARKER_PREFIX}2026-09-01T00:00:00.000Z -->` }, + { body: 'another unrelated comment' }, + ]; + assert.equal( + findFirstConflictObservedAt(comments), + '2026-09-01T00:00:00.000Z', + ); +}); + +test('findFirstConflictObservedAt: returns null when no marker exists', () => { + const comments = [{ body: 'just a regular comment' }]; + assert.equal(findFirstConflictObservedAt(comments), null); +}); + +test('hoursSince: computes elapsed hours against a fixed "now"', () => { + const now = Date.parse('2026-09-03T00:00:00.000Z'); + const hours = hoursSince('2026-09-01T00:00:00.000Z', now); + assert.equal(hours, 48); +}); + +test('hoursSince: returns a value under 48 for a recent timestamp', () => { + const now = Date.now(); + const recent = new Date(now - 10 * 60 * 60 * 1000).toISOString(); + assert.ok(hoursSince(recent, now) < 48); +}); + +test('isConflicting: true when mergeable is false, even if mergeable_state is stale', async () => { + const result = await isConflicting('owner/repo', 1, () => ({ + mergeable: false, + mergeable_state: 'unknown', + })); + assert.equal(result, true); +}); + +test('isConflicting: true when mergeable_state is dirty', async () => { + const result = await isConflicting('owner/repo', 1, () => ({ + mergeable: null, + mergeable_state: 'dirty', + })); + assert.equal(result, true); +}); + +test('isConflicting: false when mergeable is true and state is clean', async () => { + const result = await isConflicting('owner/repo', 1, () => ({ + mergeable: true, + mergeable_state: 'clean', + })); + assert.equal(result, false); +}); + +test('isConflicting: retries when mergeable is null and detects conflict on retry', async () => { + let callCount = 0; + let slept = 0; + const mockFetch = () => { + callCount += 1; + if (callCount === 1) { + return { mergeable: null, mergeable_state: 'unknown' }; + } + return { mergeable: false, mergeable_state: 'dirty' }; + }; + const mockSleep = async (ms) => { + slept += ms; + }; + const result = await isConflicting('owner/repo', 1, mockFetch, 2, mockSleep); + assert.equal(result, true); + assert.equal(callCount, 2); + assert.equal(slept, 1000); +}); + +test('isConflicting: returns false if retries exhaust without resolving conflict', async () => { + let callCount = 0; + let slept = 0; + const mockFetch = () => { + callCount += 1; + return { mergeable: null, mergeable_state: 'unknown' }; + }; + const mockSleep = async (ms) => { + slept += ms; + }; + const result = await isConflicting('owner/repo', 1, mockFetch, 2, mockSleep); + assert.equal(result, false); + assert.equal(callCount, 3); + assert.equal(slept, 2000); +}); + +test('isHeld: identifies hold, on-hold, and do-not-merge labels case-insensitively', () => { + assert.equal(isHeld({ labels: [{ name: 'HOLD' }] }), true); + assert.equal(isHeld({ labels: [{ name: 'on-hold' }] }), true); + assert.equal(isHeld({ labels: [{ name: 'do-not-merge' }] }), true); + assert.equal(isHeld({ labels: ['hold'] }), true); + assert.equal(isHeld({ labels: [{ name: 'bug' }] }), false); + assert.equal(isHeld({ labels: [] }), false); + assert.equal(isHeld({}), false); +});