From 08e04b9c75e97acc5ec394770784b5da9f1e8c32 Mon Sep 17 00:00:00 2001 From: Vineeth Voruganti <13438633+VVoruganti@users.noreply.github.com> Date: Mon, 24 Aug 2026 17:23:34 -0400 Subject: [PATCH] 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) --- .github/scripts/issue-gate.js | 51 +++++++++++++++------ .github/scripts/issue-gate.test.js | 71 +++++++++++++++++++++++++++++- 2 files changed, 107 insertions(+), 15 deletions(-) diff --git a/.github/scripts/issue-gate.js b/.github/scripts/issue-gate.js index dca13f34..c9bbb668 100644 --- a/.github/scripts/issue-gate.js +++ b/.github/scripts/issue-gate.js @@ -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, }; diff --git a/.github/scripts/issue-gate.test.js b/.github/scripts/issue-gate.test.js index af05a09d..695df3a2 100644 --- a/.github/scripts/issue-gate.test.js +++ b/.github/scripts/issue-gate.test.js @@ -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`); })();