mirror of
https://github.com/affaan-m/ECC.git
synced 2026-10-23 03:27:50 +09:00
feat(ci): cap open PRs per contributor at ten (#3639)
Extend the PR burst guard with an open-PR cap. On every opened or reopened PR by a non-collaborator, the guard lists the repository's open PRs, keeps the author's oldest `maxOpen` (10) open and closes the newer ones with a bot comment that names the count and the limit. No lock, no maintainer alert: the cap is about review capacity, not spam. PRs the burst logic already closed in the same run are excluded so nothing is mutated twice. Collaborators with write access stay exempt, dry run reports without mutating, and the comment is deduplicated on a bot-authored marker. Haley asked for this on 2026-10-19: each contributor should only have 10 to 15 PRs open at a time. Existing open PRs are not bulk-closed; the cap applies as each author opens or reopens their next PR. Verification: 22 unit tests and the HTTP workflow test pass, validate-workflow-security passes, eslint and markdownlint clean. Dry run against affaan-m/ECC on #3621 reports openCount 19 and nine cap excess PRs for that author, with no writes.
This commit is contained in:
1 parent
76aaa77df6
commit
6c557856af
6 files changed
+122
-10
No files matched your search
@@ -10,13 +10,22 @@ 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 |
|
||||
| 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 |
|
||||
|
||||
Edit `pr-burst-policy.json` to change thresholds, notification recipients or
|
||||
denied numeric IDs. This policy limits submission rate, with no limit on the
|
||||
total number of open PRs a contributor can accumulate over time.
|
||||
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
|
||||
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.
|
||||
|
||||
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
|
||||
|
||||
@@ -2,6 +2,7 @@
|
||||
"windowMinutes": 20,
|
||||
"closeAbove": 5,
|
||||
"blockAbove": 10,
|
||||
"maxOpen": 10,
|
||||
"deniedUserIDs": [108231307],
|
||||
"notify": ["affaan-m", "haelyra"]
|
||||
}
|
||||
@@ -441,6 +441,12 @@ closes and locks excess PRs, including drafts. More than ten submissions within
|
||||
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.
|
||||
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.
|
||||
|
||||
See the [moderation runbook](.github/PR_MODERATION.md)
|
||||
for activation, credential requirements, and recovery.
|
||||
|
||||
|
||||
@@ -6,7 +6,7 @@ const path = require('node:path');
|
||||
const LOGIN = /^[a-z\d](?:[a-z\d-]{0,38})(?:\[bot\])?$/i;
|
||||
|
||||
function validatePolicy(policy) {
|
||||
for (const key of ['windowMinutes', 'closeAbove', 'blockAbove']) {
|
||||
for (const key of ['windowMinutes', 'closeAbove', 'blockAbove', 'maxOpen']) {
|
||||
if (!Number.isSafeInteger(policy[key]) || policy[key] < 1) throw new Error(`Invalid policy ${key}`);
|
||||
}
|
||||
if (policy.blockAbove <= policy.closeAbove) throw new Error('blockAbove must exceed closeAbove');
|
||||
@@ -86,6 +86,33 @@ 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()) {
|
||||
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);
|
||||
return { openCount: open.length, maxOpen: policy.maxOpen,
|
||||
capExcess: open.slice(policy.maxOpen).map(pr => ({ number: pr.number, state: pr.state })) };
|
||||
}
|
||||
|
||||
const CAP_MARKER = '<!-- ecc-pr-open-cap -->';
|
||||
|
||||
async function enforceCap(api, root, pr, current, result) {
|
||||
try {
|
||||
await api('PATCH', `${root}/pulls/${pr.number}`, { state: 'closed' });
|
||||
const comments = await listPages(api, `${root}/issues/${pr.number}/comments?sort=created`);
|
||||
if (comments.some(c => c.user?.login === 'github-actions[bot]' && c.body?.includes(CAP_MARKER))) return;
|
||||
await api('POST', `${root}/issues/${pr.number}/comments`, { body: [CAP_MARKER,
|
||||
`Closed automatically: @${current.user.login} has ${result.openCount} open pull requests in this repository, above the limit of ${result.maxOpen} per contributor.`,
|
||||
'', 'Get existing PRs merged, or close the ones you no longer need, then reopen this one. The limit is about review capacity, not the quality of this patch.',
|
||||
'Contributors with repository write access are exempt. Policy: `.github/PR_MODERATION.md`.'].join('\n') });
|
||||
} catch (error) {
|
||||
result.errors.push(`#${pr.number}: ${error.message}`);
|
||||
}
|
||||
}
|
||||
|
||||
async function blockAuthor(blockAPI, owner, login) {
|
||||
if (!blockAPI) return 'Unavailable: PR_MODERATION_BLOCK_TOKEN is not configured';
|
||||
try {
|
||||
@@ -160,11 +187,19 @@ async function moderate({ api, blockAPI, repository, number, action = 'opened',
|
||||
// let an older event conceal a newer burst already present in the API snapshot.
|
||||
const result = { ...strongestWindow(rows, current, policy, end), author: current.user.login,
|
||||
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))));
|
||||
if (dryRun) {
|
||||
if (result.block) result.blockStatus = 'Dry run: would attempt owner-scoped blocking';
|
||||
return result;
|
||||
}
|
||||
if (result.excess.length === 0) return result;
|
||||
if (result.excess.length > 0) await enforceBurst({ api, blockAPI, root, repo, current, result, policy });
|
||||
for (const pr of result.capExcess) await enforceCap(api, root, pr, current, result);
|
||||
return result;
|
||||
}
|
||||
|
||||
async function enforceBurst({ api, blockAPI, root, repo, current, result, policy }) {
|
||||
if (result.block) result.blockStatus = await blockAuthor(blockAPI, repo.owner, current.user.login);
|
||||
for (const pr of result.excess) {
|
||||
if (pr.state !== 'open') continue;
|
||||
@@ -176,7 +211,6 @@ async function moderate({ api, blockAPI, repository, number, action = 'opened',
|
||||
}
|
||||
}
|
||||
await notify(api, root, current, result, policy);
|
||||
return result;
|
||||
}
|
||||
|
||||
async function main() {
|
||||
@@ -196,4 +230,4 @@ async function main() {
|
||||
}
|
||||
|
||||
if (require.main === module) main().catch(error => { console.error(error.message); process.exitCode = 1; });
|
||||
module.exports = { evaluate, moderate, githubAPI, listPages, validatePolicy, strongestWindow };
|
||||
module.exports = { evaluate, moderate, githubAPI, listPages, validatePolicy, strongestWindow, capExcess };
|
||||
@@ -40,6 +40,7 @@ test('actual CLI closes six excess PRs, locks them, blocks with owner identity,
|
||||
return send(issue);
|
||||
}
|
||||
if (url.pathname.endsWith('/issues/88') && req.method === 'PATCH') { issue = { ...issue, ...body }; return send(issue); }
|
||||
if (/\/issues\/\d+\/comments$/.test(url.pathname)) return send(req.method === 'GET' ? [] : {});
|
||||
if (req.method === 'PATCH' && /\/pulls\/\d+$/.test(url.pathname)) {
|
||||
const pr = rows.find(row => row.number === Number(url.pathname.split('/').pop()));
|
||||
pr.state = body.state; return send(pr);
|
||||
|
||||
@@ -1,9 +1,9 @@
|
||||
#!/usr/bin/env node
|
||||
const assert = require('node:assert/strict');
|
||||
const { test } = require('node:test');
|
||||
const { evaluate, moderate, githubAPI, listPages, strongestWindow } = require('../../scripts/ci/pr-burst-guard');
|
||||
const { evaluate, moderate, githubAPI, listPages, strongestWindow, capExcess } = require('../../scripts/ci/pr-burst-guard');
|
||||
|
||||
const policy = { windowMinutes: 20, closeAbove: 5, blockAbove: 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 });
|
||||
@@ -18,14 +18,15 @@ function fixture(n, options = {}) {
|
||||
if (route === '/repos/owner/repo') return { owner: { login: 'owner', type: 'User' } };
|
||||
if (route.endsWith(`/pulls/${n}`)) return rows.find(p => p.number === n);
|
||||
if (route.includes('/collaborators/')) return { permission: options.permission || 'read' };
|
||||
if (route.includes('/pulls?')) return rows.slice().reverse();
|
||||
if (route.includes('/pulls?')) return rows.filter(p => !route.includes('state=open') || p.state === 'open').slice().reverse();
|
||||
if (route.includes('/issues?')) return issue ? [issue] : [];
|
||||
if (/\/issues\/\d+\/comments\?/.test(route) && method === 'GET') return options.comments || [];
|
||||
if (route.endsWith('/issues') && method === 'POST') {
|
||||
issue = { number: 101, user: { login: 'github-actions[bot]' }, ...body };
|
||||
return issue;
|
||||
}
|
||||
if (route.endsWith('/issues/101') && method === 'PATCH') { issue = { ...issue, ...body }; return issue; }
|
||||
if (route.endsWith('/issues/101/comments')) return {};
|
||||
if (route.endsWith('/comments') && method === 'POST') return {};
|
||||
if (method === 'PATCH') return {};
|
||||
if (method === 'PUT') return {};
|
||||
throw new Error(`Unexpected ${method} ${route}`);
|
||||
@@ -178,6 +179,66 @@ test('closure failure still creates an alert with enforcement errors', async ()
|
||||
assert(f.calls.some(c => c.method === 'POST' && c.body.body.includes('close denied')));
|
||||
});
|
||||
|
||||
// Spread PRs one hour apart so the burst window never fires; only the open-PR cap applies.
|
||||
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', () => {
|
||||
const result = capExcess(spread(12), spread(12)[11], policy);
|
||||
assert.equal(result.openCount, 12);
|
||||
assert.deepEqual(result.capExcess.map(p => p.number), [11, 12]);
|
||||
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.equal(capExcess(spread(12, { user: { id: 7, login: 'other' } }), pull(1), policy).openCount, 1);
|
||||
});
|
||||
|
||||
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 });
|
||||
const result = await moderate(f.args);
|
||||
assert.equal(result.count, 1);
|
||||
assert.deepEqual(result.capExcess.map(p => p.number), [11]);
|
||||
assert(f.calls.some(c => c.method === 'PATCH' && c.route.endsWith('/pulls/11') && c.body.state === 'closed'));
|
||||
const comment = f.calls.find(c => c.method === 'POST' && c.route.endsWith('/issues/11/comments'));
|
||||
assert.match(comment.body.body, /11 open pull requests.*limit of 10/);
|
||||
assert.equal(f.calls.filter(c => c.route.endsWith('/lock')).length, 0);
|
||||
assert.equal(f.calls.filter(c => c.method === 'POST' && c.route.endsWith('/issues')).length, 0);
|
||||
const again = fixture(11, { rows, comments: [{ user: { login: 'github-actions[bot]' }, body: comment.body.body }] });
|
||||
await moderate(again.args);
|
||||
assert.equal(again.calls.filter(c => c.method === 'POST').length, 0);
|
||||
// A contributor-authored lookalike comment cannot suppress the explanation.
|
||||
const spoofed = fixture(11, { rows, comments: [{ user: { login: 'contributor' }, body: comment.body.body }] });
|
||||
await moderate(spoofed.args);
|
||||
assert.equal(spoofed.calls.filter(c => c.method === 'POST').length, 1);
|
||||
});
|
||||
|
||||
test('cap counts open PRs after burst closures, so one run never closes a PR twice', async () => {
|
||||
const rows = [...spread(9), ...pulls(6).map(p => ({ ...p, number: p.number + 9,
|
||||
created_at: new Date(start + 24 * 3600000 + p.number * 1000).toISOString() }))];
|
||||
const f = fixture(15, { rows });
|
||||
const result = await moderate(f.args);
|
||||
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.equal(f.calls.filter(c => c.method === 'PATCH' && c.route.endsWith('/pulls/15')).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(f.calls.every(c => c.method === 'GET'));
|
||||
const w = fixture(12, { rows: spread(12), permission: 'write' });
|
||||
assert.equal((await moderate(w.args)).exempt, true);
|
||||
assert.equal(w.calls.filter(c => c.method !== 'GET').length, 0);
|
||||
});
|
||||
|
||||
test('policy requires a positive integer maxOpen', () => {
|
||||
assert.throws(() => capExcess([], pull(1), { ...policy, maxOpen: 0 }), /maxOpen/);
|
||||
assert.throws(() => capExcess([], pull(1), { ...policy, maxOpen: undefined }), /maxOpen/);
|
||||
});
|
||||
|
||||
test('reports only moderation metadata, never PR bodies or titles', async () => {
|
||||
const f = fixture(6, { rows: pulls(6).map(p => ({ ...p, title: 'untrusted title', body: 'untrusted body' })) });
|
||||
assert.doesNotMatch(JSON.stringify(await moderate(f.args)), /untrusted/);
|
||||
|
||||
Reference in new issue
Block a user