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 f99e7042..0233451a 100644 --- a/package.json +++ b/package.json @@ -61,6 +61,7 @@ "validate:radar-reports": "node scripts/validate-radar-reports.mjs", "validate:community-people": "node scripts/validate-community-people.mjs", "validate:community-groups": "node scripts/validate-community-groups.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); +});