From 4062ffbbbe21cdcb86d55ccd1500afa8a9a63476 Mon Sep 17 00:00:00 2001 From: haelyra <49814733+haelyra@users.noreply.github.com> Date: Tue, 20 Oct 2026 16:15:38 -0400 Subject: [PATCH] fix(ci): grandfather open PRs under contributor cap --- .github/PR_MODERATION.md | 25 ++++++----- .github/workflows/pr-burst-guard.yml | 5 ++- CONTRIBUTING.md | 5 ++- scripts/ci/pr-burst-guard.js | 13 +++--- tests/ci/pr-burst-guard-workflow.test.js | 57 +++++++++++++++++++++++- tests/ci/pr-burst-guard.test.js | 51 ++++++++++++++++++--- 6 files changed, 128 insertions(+), 28 deletions(-) diff --git a/.github/PR_MODERATION.md b/.github/PR_MODERATION.md index 27b13f5cc..949ab92c6 100644 --- a/.github/PR_MODERATION.md +++ b/.github/PR_MODERATION.md @@ -10,7 +10,7 @@ judge whether a patch is useful. Existing open PRs are not bulk-closed on rollou | --- | --- | --- | | Burst allowance | Five creations per author in 20 minutes | Close and lock excess PRs | | Severe burst | More than ten creations in 20 minutes | Attempt account block; alert maintainers | -| Open-PR cap | Ten open PRs per author at any time | Close the newest PRs above the cap with an explanatory comment; no lock, no alert | +| Open-PR cap | Ten open PRs per author at any time | Close only the newly opened or reopened PR above the cap with an explanatory comment; no lock, no alert | | Known denied account | User ID `108231307`, currently `dajiaohuang` | Close and lock its incoming/reopened PR | | Trusted contributors | Live repository write, maintain, or admin permission | Exempt from the burst guard | | Notifications | `@affaan-m @haelyra` | One bot-authored issue per author per UTC day, plus one escalation comment | @@ -18,14 +18,20 @@ judge whether a patch is useful. Existing open PRs are not bulk-closed on rollou Edit `pr-burst-policy.json` to change thresholds, notification recipients or denied numeric IDs, or `maxOpen` to change the open-PR cap. -The open-PR cap is evaluated on the same opened and reopened events, after any -burst closures in that run, against a full paginated list of the repository's -open PRs. The author's oldest `maxOpen` PRs stay open; newer ones are closed with -a bot comment that names the count and the limit. Closing is reversible: once -the author is under the cap, reopening a closed PR passes. A PR the burst guard +The open-PR cap runs on opened and reopened events. Up to 100 author events +queue in Actions, so normal submission bursts keep their pending cap checks. +The guard checks a full paginated list of open PRs. A newly opened PR closes +only when at least ten older PRs by that author are still open. +Reopening consumes a new slot, so the reopened PR closes when the author is +over the cap. Existing open PRs stay open, even when their author already had +more than ten. A bot comment names the count and limit. Closing is reversible: +once the author is under the cap, reopening a closed PR passes. A PR the burst guard already closed and locked in the same run is never touched again by the cap. The -cap does not bulk-close PRs that were already open when it shipped; it applies -as each author opens or reopens their next PR. +cap does not bulk-close historical PRs; it applies when an author opens or +reopens a PR after rollout. Burst moderation runs before the cap in the same +job and still reads later creation windows to handle out-of-order API snapshots. +GitHub cancels excess pending runs beyond its 100-run queue; a prolonged backlog +that reaches that limit can leave an individual cap event unchecked. Closed and draft PRs count toward burst detection. Counting uses stable author IDs and paginated REST creation metadata, without the search index. A run checks @@ -33,8 +39,7 @@ historical windows from the triggering PR's creation time through newer API rows. Queue delays, out-of-order delivery and replaced pending runs cannot hide a burst already present in that snapshot. Excess PRs are determined in creation order across all scanned windows, with PR number breaking timestamp ties. -Already closed excess PRs count -but are not mutated again. +Already closed excess PRs count but are not mutated again. Reopening an old ordinary PR does not add a new creation to the burst count. Reopening a denied author's PR triggers denial even outside the time window. diff --git a/.github/workflows/pr-burst-guard.yml b/.github/workflows/pr-burst-guard.yml index 550655339..fbb19ecf3 100644 --- a/.github/workflows/pr-burst-guard.yml +++ b/.github/workflows/pr-burst-guard.yml @@ -4,11 +4,12 @@ on: pull_request_target: types: [opened, reopened] -# Process one author's ledger serially. A newer pending event replaces an older -# pending event; each run reads all PR states instead of relying on event counts. +# Queue up to 100 pending events per author. The burst scan still handles +# delayed or out-of-order API snapshots. concurrency: group: pr-burst-${{ github.repository }}-${{ github.event.pull_request.user.id }} cancel-in-progress: false + queue: max permissions: contents: read diff --git a/CONTRIBUTING.md b/CONTRIBUTING.md index 8fe695388..70790a656 100644 --- a/CONTRIBUTING.md +++ b/CONTRIBUTING.md @@ -442,8 +442,9 @@ still count toward the window. Contributors with repository write access bypass this policy. Maintainers can review exceptions through the moderation alert. Please also keep at most ten PRs open at a time. When you open or reopen a PR -above that cap, automation closes the newest ones with a comment explaining the -count. Get existing PRs merged or close the ones you no longer need, then reopen. +above that cap, automation closes that PR with a comment explaining the count. +Existing open PRs remain open. Get PRs merged or close the ones you no longer +need, then reopen. Ten open PRs is more than any one reviewer can keep in flight, so this keeps review time on the work you care about most. diff --git a/scripts/ci/pr-burst-guard.js b/scripts/ci/pr-burst-guard.js index d7f3f8653..db8fbfc5f 100644 --- a/scripts/ci/pr-burst-guard.js +++ b/scripts/ci/pr-burst-guard.js @@ -86,15 +86,18 @@ function strongestWindow(rows, current, policy, eventTime) { return { ...peak, excess: peak.denied ? peak.excess : [...excess.values()] }; } -/** Open-PR cap: keep the author's oldest `maxOpen` open PRs, close the rest. PRs the burst - * guard already closed in this run are treated as closed so nothing is mutated twice. */ -function capExcess(openRows, current, policy, alreadyClosed = new Set()) { +/** Only the triggering PR may be closed by the cap. Reopening consumes a new + * slot even if that PR is older than the author's other open PRs. */ +function capExcess(openRows, current, policy, alreadyClosed = new Set(), action = 'opened') { validatePolicy(policy); const open = [...new Map([...openRows, current].map(pr => [pr.number, pr])).values()] .filter(pr => pr.user.id === current.user.id && pr.state === 'open' && !alreadyClosed.has(pr.number)) .sort((a, b) => Date.parse(a.created_at) - Date.parse(b.created_at) || a.number - b.number); + const currentIndex = open.findIndex(pr => pr.number === current.number); return { openCount: open.length, maxOpen: policy.maxOpen, - capExcess: open.slice(policy.maxOpen).map(pr => ({ number: pr.number, state: pr.state })) }; + capExcess: open.length > policy.maxOpen && currentIndex >= 0 + && (action === 'reopened' || currentIndex >= policy.maxOpen) + ? [{ number: current.number, state: current.state }] : [] }; } const CAP_MARKER = ''; @@ -189,7 +192,7 @@ async function moderate({ api, blockAPI, repository, number, action = 'opened', dryRun, errors: [], blockStatus: 'Below blocking threshold' }; // Total open PRs is a separate, unwindowed list: the burst scan stops at the window edge. const openRows = await listPages(api, `${root}/pulls?state=open&sort=created&direction=desc`); - Object.assign(result, capExcess(openRows, current, policy, new Set(result.excess.map(pr => pr.number)))); + Object.assign(result, capExcess(openRows, current, policy, new Set(result.excess.map(pr => pr.number)), action)); if (dryRun) { if (result.block) result.blockStatus = 'Dry run: would attempt owner-scoped blocking'; return result; diff --git a/tests/ci/pr-burst-guard-workflow.test.js b/tests/ci/pr-burst-guard-workflow.test.js index 0ebe1f82b..e4ce66cef 100644 --- a/tests/ci/pr-burst-guard-workflow.test.js +++ b/tests/ci/pr-burst-guard-workflow.test.js @@ -10,6 +10,10 @@ const { promisify } = require('node:util'); const exec = promisify(execFile); const root = path.resolve(__dirname, '../..'); +function recordMutation(mutations, req, route, body) { + if (req.method !== 'GET') mutations.push({ method: req.method, route, body, token: req.headers.authorization }); +} + test('actual CLI closes six excess PRs, locks them, blocks with owner identity, and deduplicates alerts over HTTP', async () => { const tmp = fs.mkdtempSync(path.join(os.tmpdir(), 'ecc-pr-guard-')); const mutations = []; @@ -25,7 +29,7 @@ test('actual CLI closes six excess PRs, locks them, blocks with owner identity, for await (const chunk of req) text += chunk; const body = text ? JSON.parse(text) : undefined; const send = value => { res.setHeader('Content-Type', 'application/json'); res.end(JSON.stringify(value)); }; - if (req.method !== 'GET') mutations.push({ method: req.method, route: url.pathname, body, token: req.headers.authorization }); + recordMutation(mutations, req, url.pathname, body); if (url.pathname === '/repos/owner/repo') return send({ owner: { login: 'owner', type: 'User' } }); if (url.pathname === '/user') { assert.equal(req.headers.authorization, 'Bearer owner-token'); @@ -67,7 +71,7 @@ test('actual CLI closes six excess PRs, locks them, blocks with owner identity, await run(); assert.equal(mutations.filter(m => m.route.endsWith('/issues') && m.method === 'POST').length, 1); assert.equal(mutations.filter(m => /\/pulls\/\d+$/.test(m.route)).length, 6); - assert.equal(mutations.filter(m => m.route.endsWith('/lock')).length, 6); + assert.equal(new Set(mutations.filter(m => m.route.endsWith('/lock')).map(m => m.route)).size, 6); assert(mutations.some(m => m.route === '/user/blocks/contributor' && m.token === 'Bearer owner-token')); assert(mutations.filter(m => m.route.startsWith('/repos/')).every(m => m.token === 'Bearer moderation-token')); assert.doesNotMatch(fs.readFileSync(summary, 'utf8'), /untrusted|owner-token|moderation-token/); @@ -86,3 +90,52 @@ test('actual CLI closes six excess PRs, locks them, blocks with owner identity, fs.rmSync(tmp, { recursive: true, force: true }); } }); + +test('actual CLI closes only a new PR when twenty earlier PRs remain open', async () => { + const tmp = fs.mkdtempSync(path.join(os.tmpdir(), 'ecc-pr-cap-')); + const rows = Array.from({ length: 21 }, (_, i) => ({ number: i === 20 ? 3649 : i + 1, + user: { id: 42, login: 'contributor' }, state: 'open', + created_at: new Date(Date.UTC(2026, 9, 1, i, 0, 0)).toISOString() })); + const mutations = []; + const server = http.createServer(async (req, res) => { + const url = new URL(req.url, 'http://localhost'); + let text = ''; + for await (const chunk of req) text += chunk; + const body = text ? JSON.parse(text) : undefined; + const send = value => { res.setHeader('Content-Type', 'application/json'); res.end(JSON.stringify(value)); }; + if (req.method !== 'GET') mutations.push({ method: req.method, route: url.pathname, body }); + if (url.pathname === '/repos/owner/repo') return send({ owner: { login: 'owner', type: 'User' } }); + if (url.pathname.includes('/collaborators/')) return send({ permission: 'read' }); + if (url.pathname.endsWith('/pulls/3649') && req.method === 'GET') return send(rows[20]); + if (url.pathname.endsWith('/pulls')) return send(rows.filter(pr => url.searchParams.get('state') !== 'open' || pr.state === 'open').reverse()); + if (url.pathname.endsWith('/issues/3649/comments')) return send(req.method === 'GET' ? [] : {}); + if (url.pathname.endsWith('/pulls/3649') && req.method === 'PATCH') { rows[20].state = body.state; return send(rows[20]); } + res.statusCode = 404; send({ message: 'fixture route missing' }); + }); + await new Promise(resolve => server.listen(0, '127.0.0.1', resolve)); + try { + const port = server.address().port; + const preload = path.join(tmp, 'transport.cjs'); + fs.writeFileSync(preload, `const original = global.fetch; global.fetch = (url, options) => original('http://127.0.0.1:${port}' + new URL(url).pathname + new URL(url).search, options);`); + const event = path.join(tmp, 'event.json'); + fs.writeFileSync(event, JSON.stringify({ action: 'opened', pull_request: { number: 3649 } })); + const env = { ...process.env, NODE_OPTIONS: `--require=${preload}`, GITHUB_REPOSITORY: 'owner/repo', + GITHUB_TOKEN: 'moderation-token', GITHUB_EVENT_PATH: event, PR_MODERATION_BLOCK_TOKEN: '' }; + const report = JSON.parse((await exec(process.execPath, ['scripts/ci/pr-burst-guard.js'], { cwd: root, env })).stdout); + assert.equal(report.openCount, 21); + assert.deepEqual(report.capExcess.map(pr => pr.number), [3649]); + assert.equal(rows.filter(pr => pr.state === 'open').length, 20); + assert.deepEqual(mutations.filter(m => m.method === 'PATCH').map(m => m.route), ['/repos/owner/repo/pulls/3649']); + assert.match(mutations.find(m => m.route.endsWith('/issues/3649/comments')).body.body, /21 open pull requests.*limit of 10/); + } finally { + await new Promise(resolve => server.close(resolve)); + fs.rmSync(tmp, { recursive: true, force: true }); + } +}); + +test('workflow requests the bounded author queue for pending cap checks', () => { + const workflow = fs.readFileSync(path.join(root, '.github/workflows/pr-burst-guard.yml'), 'utf8'); + assert.match(workflow, /group: pr-burst-.*github\.event\.pull_request\.user\.id/); + assert.match(workflow, /concurrency:[\s\S]*?queue: max/); + assert.match(workflow, /run: node scripts\/ci\/pr-burst-guard\.js/); +}); diff --git a/tests/ci/pr-burst-guard.test.js b/tests/ci/pr-burst-guard.test.js index 36902dc4e..48ef9be89 100644 --- a/tests/ci/pr-burst-guard.test.js +++ b/tests/ci/pr-burst-guard.test.js @@ -3,7 +3,8 @@ const assert = require('node:assert/strict'); const { test } = require('node:test'); const { evaluate, moderate, githubAPI, listPages, strongestWindow, capExcess } = require('../../scripts/ci/pr-burst-guard'); -const policy = { windowMinutes: 20, closeAbove: 5, blockAbove: 10, maxOpen: 10, deniedUserIDs: [999], notify: ['maintainer'] }; +const policy = { windowMinutes: 20, closeAbove: 5, blockAbove: 10, maxOpen: 10, + deniedUserIDs: [999], notify: ['maintainer'] }; const start = Date.parse('2026-10-14T19:00:00Z'); const pull = (number, extra = {}) => ({ number, user: { id: 42, login: 'contributor' }, created_at: new Date(start + number * 1000).toISOString(), state: 'open', ...extra }); @@ -16,7 +17,8 @@ function fixture(n, options = {}) { const api = async (method, route, body) => { calls.push({ method, route, body }); if (route === '/repos/owner/repo') return { owner: { login: 'owner', type: 'User' } }; - if (route.endsWith(`/pulls/${n}`)) return rows.find(p => p.number === n); + const pullNumber = route.match(/\/pulls\/(\d+)$/); + if (pullNumber && method === 'GET') return rows.find(p => p.number === Number(pullNumber[1])); if (route.includes('/collaborators/')) return { permission: options.permission || 'read' }; if (route.includes('/pulls?')) return rows.filter(p => !route.includes('state=open') || p.state === 'open').slice().reverse(); if (route.includes('/issues?')) return issue ? [issue] : []; @@ -183,16 +185,50 @@ test('closure failure still creates an alert with enforcement errors', async () const spread = (n, extra = {}) => Array.from({ length: n }, (_, i) => pull(i + 1, { created_at: new Date(start + i * 3600000).toISOString(), ...extra })); -test('open-PR cap keeps the oldest ten open and closes the rest, newest first', () => { +test('open-PR cap closes only the triggering excess PR', () => { + assert.deepEqual(capExcess(spread(10), spread(10)[9], policy).capExcess, []); + assert.deepEqual(capExcess(spread(11), spread(11)[10], policy).capExcess.map(p => p.number), [11]); const result = capExcess(spread(12), spread(12)[11], policy); assert.equal(result.openCount, 12); - assert.deepEqual(result.capExcess.map(p => p.number), [11, 12]); + assert.deepEqual(result.capExcess.map(p => p.number), [12]); + const grandfathered = capExcess(spread(21), spread(21)[20], policy); + assert.equal(grandfathered.openCount, 21); + assert.deepEqual(grandfathered.capExcess.map(p => p.number), [21]); + assert.deepEqual(capExcess(spread(11), spread(11)[0], policy, new Set(), 'reopened') + .capExcess.map(p => p.number), [1]); const closed = spread(12).map((p, i) => i < 3 ? { ...p, state: 'closed' } : p); assert.equal(capExcess(closed, closed[11], policy).openCount, 9); - assert.equal(capExcess(spread(12), spread(12)[11], policy, new Set([11, 12])).capExcess.length, 0); + assert.deepEqual(capExcess(spread(12), spread(12)[11], policy, new Set([12])).capExcess, []); assert.equal(capExcess(spread(12, { user: { id: 7, login: 'other' } }), pull(1), policy).openCount, 1); }); +test('a new PR over twenty existing open PRs is the only cap closure and gets a comment', async () => { + const f = fixture(21, { rows: spread(21) }); + const result = await moderate(f.args); + assert.deepEqual(result.capExcess.map(p => p.number), [21]); + assert.deepEqual(f.calls.filter(c => c.method === 'PATCH' && c.route.includes('/pulls/')).map(c => c.route), + ['/repos/owner/repo/pulls/21']); + const comment = f.calls.find(c => c.method === 'POST' && c.route.endsWith('/issues/21/comments')); + assert.match(comment.body.body, /21 open pull requests.*limit of 10/); +}); + +test('reopening an older PR over the cap closes that PR without touching its peers', async () => { + const f = fixture(11, { rows: spread(11), action: 'reopened' }); + f.args.number = 1; + const result = await moderate(f.args); + assert.deepEqual(result.capExcess.map(p => p.number), [1]); + assert.deepEqual(f.calls.filter(c => c.method === 'PATCH' && c.route.includes('/pulls/')).map(c => c.route), + ['/repos/owner/repo/pulls/1']); +}); + +test('an older allowed PR stays open when later PRs are already visible', async () => { + const f = fixture(12, { rows: spread(12) }); + f.args.number = 10; + const result = await moderate(f.args); + assert.deepEqual(result.capExcess, []); + assert.equal(f.calls.filter(c => c.method !== 'GET').length, 0); +}); + test('cap closes with an explanatory comment, no lock, no maintainer alert, and never comments twice', async () => { const rows = spread(11); const f = fixture(11, { rows }); @@ -221,13 +257,14 @@ test('cap counts open PRs after burst closures, so one run never closes a PR twi assert.equal(result.count, 6); assert.deepEqual(result.excess.map(p => p.number), [15]); assert.equal(result.openCount, 14); - assert.deepEqual(result.capExcess.map(p => p.number), [11, 12, 13, 14]); + assert.deepEqual(result.capExcess, []); assert.equal(f.calls.filter(c => c.method === 'PATCH' && c.route.endsWith('/pulls/15')).length, 1); + assert.equal(f.calls.filter(c => c.method === 'PATCH' && c.route.includes('/pulls/')).length, 1); }); test('cap dry run reports the excess without mutating; collaborators are exempt from the cap', async () => { const f = fixture(12, { rows: spread(12), dryRun: true }); - assert.deepEqual((await moderate(f.args)).capExcess.map(p => p.number), [11, 12]); + assert.deepEqual((await moderate(f.args)).capExcess.map(p => p.number), [12]); assert(f.calls.every(c => c.method === 'GET')); const w = fixture(12, { rows: spread(12), permission: 'write' }); assert.equal((await moderate(w.args)).exempt, true);