mirror of
https://github.com/affaan-m/ECC.git
synced 2026-11-07 08:35:54 +00:00
fix(ci): grandfather open PRs under contributor cap
This commit is contained in:
6 files changed
+128
-28
No files matched your search
+15
-10
@@ -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.
|
||||
|
||||
@@ -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
|
||||
|
||||
+3
-2
@@ -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.
|
||||
|
||||
|
||||
@@ -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 = '<!-- ecc-pr-open-cap -->';
|
||||
@@ -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;
|
||||
|
||||
@@ -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/);
|
||||
});
|
||||
@@ -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);
|
||||
|
||||
Reference in new issue
Block a user