fix(ci): count only bot-authored gate notices, share the exemption list
Two review findings on the issue gate, with a common root cause. MARKER is an invisible HTML comment, so anyone who can comment on a public repository can paste it. findNotices accepted any comment containing it, so a third party could post one on someone else's pull request: runGate posts a notice only when none exists, so the author would never be told, and runSweep would then measure the 72-hour grace window from the stranger's timestamp and close them unwarned. Notices now require bot authorship. The stale-draft sweep re-listed the gate's exemptions and had lost the bot case, so a bot's long-lived draft was closable despite checkGate exempting bots. Both callers now share one exemptReason(pr) rather than keeping parallel lists that drift. Not changed: closingIssuesReferences(first: 20) truncation. It needs a pull request with 21+ closing references where only a later one carries the label, and the outcome would be a label plus the grace window, not a close. Coverage goes 11 -> 20 cases, including the stale-draft close path, which had none. Both fixes were confirmed to fail their tests when reverted. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
This commit is contained in:
parent
7f2134827c
commit
08e04b9c75
|
|
@ -27,6 +27,24 @@ const DRAFT_STALE_DAYS = 30;
|
|||
|
||||
const hasLabel = (pr, name) => (pr.labels || []).some((l) => l.name === name);
|
||||
|
||||
const isBot = (account) => Boolean(account) && account.type === 'Bot';
|
||||
|
||||
/**
|
||||
* Why this pull request is exempt from the gate, or null if it is not.
|
||||
*
|
||||
* Single source of truth: every caller that acts on a pull request runs this.
|
||||
* The stale-draft sweep previously re-listed these checks and silently lost the
|
||||
* bot case.
|
||||
*/
|
||||
const exemptReason = (pr) => {
|
||||
if (isBot(pr.user)) return 'author is a bot';
|
||||
if (WRITE_ACCESS.includes(pr.author_association)) {
|
||||
return `author_association is ${pr.author_association}`;
|
||||
}
|
||||
if (hasLabel(pr, EXEMPT_LABEL)) return `carries the ${EXEMPT_LABEL} label`;
|
||||
return null;
|
||||
};
|
||||
|
||||
// Write access to the repository. CONTRIBUTOR is deliberately absent: GitHub uses
|
||||
// it for "has previously committed to the repository", which describes every
|
||||
// returning outside contributor, not a maintainer. Do not add it.
|
||||
|
|
@ -60,13 +78,8 @@ const CLOSING_ISSUES = `
|
|||
async function checkGate({ github, owner, repo, pr }) {
|
||||
if (pr.state !== 'open') return { passed: true, skipped: 'pull request is not open' };
|
||||
if (pr.draft) return { passed: true, skipped: 'pull request is a draft' };
|
||||
if (pr.user && pr.user.type === 'Bot') return { passed: true, skipped: 'author is a bot' };
|
||||
if (WRITE_ACCESS.includes(pr.author_association)) {
|
||||
return { passed: true, skipped: `author_association is ${pr.author_association}` };
|
||||
}
|
||||
if (hasLabel(pr, EXEMPT_LABEL)) {
|
||||
return { passed: true, skipped: `carries the ${EXEMPT_LABEL} label` };
|
||||
}
|
||||
const exempt = exemptReason(pr);
|
||||
if (exempt) return { passed: true, skipped: exempt };
|
||||
|
||||
const data = await github.graphql(CLOSING_ISSUES, { owner, repo, number: pr.number });
|
||||
const issues = data.repository.pullRequest.closingIssuesReferences.nodes;
|
||||
|
|
@ -112,12 +125,21 @@ function noticeBody({ owner, repo, reason }) {
|
|||
].join('\n');
|
||||
}
|
||||
|
||||
/** Every gate notice on a pull request, oldest first. */
|
||||
/**
|
||||
* Every gate notice this bot posted on a pull request, oldest first.
|
||||
*
|
||||
* Authorship is part of the test, not decoration. MARKER is an invisible HTML
|
||||
* comment, so anyone who can comment on a public repository can paste it. If
|
||||
* user comments counted, a third party could post one on someone else's pull
|
||||
* request: `runGate` posts a notice only when none exists, so the author would
|
||||
* never be told, and `runSweep` would then measure the grace window from the
|
||||
* stranger's timestamp and close them unwarned.
|
||||
*/
|
||||
async function findNotices({ github, owner, repo, number }) {
|
||||
const comments = await github.paginate(github.rest.issues.listComments, {
|
||||
owner, repo, issue_number: number, per_page: 100,
|
||||
});
|
||||
return comments.filter((c) => (c.body || '').includes(MARKER));
|
||||
return comments.filter((c) => isBot(c.user) && (c.body || '').includes(MARKER));
|
||||
}
|
||||
|
||||
/**
|
||||
|
|
@ -227,10 +249,13 @@ async function runSweep({ github, core, context, dryRun }) {
|
|||
}
|
||||
|
||||
// Stale drafts. The gate skips drafts entirely, so they never carry the label;
|
||||
// this pass keys off inactivity and re-applies the exemptions itself.
|
||||
// this pass keys off inactivity and applies the shared exemptions itself.
|
||||
for (const pr of prs.filter((p) => p.draft)) {
|
||||
if (WRITE_ACCESS.includes(pr.author_association)) continue;
|
||||
if (hasLabel(pr, EXEMPT_LABEL)) continue;
|
||||
const exempt = exemptReason(pr);
|
||||
if (exempt) {
|
||||
core.info(`#${pr.number}: leaving stale draft alone — ${exempt}`);
|
||||
continue;
|
||||
}
|
||||
|
||||
const days = (Date.now() - Date.parse(pr.updated_at)) / 86_400_000;
|
||||
if (days < DRAFT_STALE_DAYS) continue;
|
||||
|
|
@ -242,6 +267,6 @@ async function runSweep({ github, core, context, dryRun }) {
|
|||
}
|
||||
|
||||
module.exports = {
|
||||
checkGate, runGate, runSweep, noticeBody,
|
||||
checkGate, runGate, runSweep, noticeBody, findNotices, exemptReason,
|
||||
REQUIRED_LABEL, GATE_LABEL, EXEMPT_LABEL, MARKER, GRACE_HOURS, DRAFT_STALE_DAYS,
|
||||
};
|
||||
|
|
|
|||
|
|
@ -6,7 +6,9 @@
|
|||
// exercised against the real API via `pr-sweeper.yml`'s dry_run dispatch.
|
||||
|
||||
const assert = require('node:assert');
|
||||
const { checkGate, REQUIRED_LABEL, EXEMPT_LABEL } = require('./issue-gate.js');
|
||||
const {
|
||||
checkGate, findNotices, runSweep, REQUIRED_LABEL, EXEMPT_LABEL, MARKER,
|
||||
} = require('./issue-gate.js');
|
||||
|
||||
const pull = (over = {}) => ({
|
||||
number: 1, state: 'open', draft: false,
|
||||
|
|
@ -56,8 +58,73 @@ const cases = [
|
|||
(r) => r.passed === false],
|
||||
];
|
||||
|
||||
// --- findNotices: only the bot's own notices count -------------------------
|
||||
// A stranger pasting the invisible MARKER into a comment must not suppress the
|
||||
// notice or become the grace-window clock.
|
||||
const commentsStub = (comments) => ({
|
||||
paginate: async () => comments,
|
||||
rest: { issues: { listComments: null } },
|
||||
});
|
||||
|
||||
const noticeCases = [
|
||||
['a user comment carrying MARKER is not a notice',
|
||||
[{ id: 1, user: { type: 'User' }, body: `sneaky ${MARKER}`, created_at: 'x' }], 0],
|
||||
['a bot comment carrying MARKER is a notice',
|
||||
[{ id: 2, user: { type: 'Bot' }, body: `${MARKER}\nnotice`, created_at: 'x' }], 1],
|
||||
['a bot comment without MARKER is not a notice',
|
||||
[{ id: 3, user: { type: 'Bot' }, body: 'unrelated', created_at: 'x' }], 0],
|
||||
['a user MARKER does not mask the real bot notice',
|
||||
[{ id: 4, user: { type: 'User' }, body: MARKER, created_at: 'x' },
|
||||
{ id: 5, user: { type: 'Bot' }, body: MARKER, created_at: 'y' }], 1],
|
||||
];
|
||||
|
||||
// --- runSweep: the stale-draft pass must honour every exemption ------------
|
||||
const draft = (over) => ({
|
||||
number: 9, draft: true, state: 'open', labels: [],
|
||||
user: { type: 'User' }, author_association: 'NONE',
|
||||
updated_at: new Date(Date.now() - 400 * 86400_000).toISOString(),
|
||||
...over,
|
||||
});
|
||||
|
||||
async function sweepClosed(pr) {
|
||||
const closed = [];
|
||||
const github = {
|
||||
paginate: async (route) => (route === 'pulls' ? [pr] : []),
|
||||
rest: {
|
||||
pulls: {
|
||||
list: 'pulls',
|
||||
update: async ({ pull_number }) => closed.push(pull_number),
|
||||
},
|
||||
issues: { listComments: 'comments', createComment: async () => {} },
|
||||
},
|
||||
};
|
||||
await runSweep({
|
||||
github, core: { info() {}, warning() {} },
|
||||
context: { repo: { owner: 'o', repo: 'r' } }, dryRun: false,
|
||||
});
|
||||
return closed;
|
||||
}
|
||||
|
||||
const sweepCases = [
|
||||
['stale draft from an outside author closes', draft({}), 1],
|
||||
['stale draft from a bot is left alone', draft({ user: { type: 'Bot' } }), 0],
|
||||
['stale draft from a maintainer is left alone', draft({ author_association: 'MEMBER' }), 0],
|
||||
[`stale draft with ${EXEMPT_LABEL} is left alone`, draft({ labels: [{ name: EXEMPT_LABEL }] }), 0],
|
||||
['recent draft is left alone', draft({ updated_at: new Date().toISOString() }), 0],
|
||||
];
|
||||
|
||||
(async () => {
|
||||
let failed = 0;
|
||||
for (const [name, comments, want] of noticeCases) {
|
||||
const got = (await findNotices({ github: commentsStub(comments), owner: 'o', repo: 'r', number: 1 })).length;
|
||||
if (got === want) console.log(` ok ${name}`);
|
||||
else { failed++; console.log(` FAIL ${name} -> ${got} notices, wanted ${want}`); }
|
||||
}
|
||||
for (const [name, pr, want] of sweepCases) {
|
||||
const got = (await sweepClosed(pr)).length;
|
||||
if (got === want) console.log(` ok ${name}`);
|
||||
else { failed++; console.log(` FAIL ${name} -> closed ${got}, wanted ${want}`); }
|
||||
}
|
||||
for (const [name, thunk, ok] of cases) {
|
||||
const result = await thunk();
|
||||
if (ok(result)) {
|
||||
|
|
@ -68,5 +135,5 @@ const cases = [
|
|||
}
|
||||
}
|
||||
assert.strictEqual(failed, 0, `${failed} case(s) failed`);
|
||||
console.log(`\n${cases.length} passed`);
|
||||
console.log(`\n${cases.length + noticeCases.length + sweepCases.length} passed`);
|
||||
})();
|
||||
|
|
|
|||
Loading…
Reference in New Issue