diff --git a/.github/scripts/auto-minimize-spam.test.mjs b/.github/scripts/auto-minimize-spam.test.mjs deleted file mode 100644 index 12a53861492..00000000000 --- a/.github/scripts/auto-minimize-spam.test.mjs +++ /dev/null @@ -1,73 +0,0 @@ -// Regression guards for the security-critical invariants of the -// auto-minimize-spam workflow. Follows the pattern established by -// qwen-triage-workflow.test.mjs: a future edit that removes the repository -// guard, widens permissions, moves GH_TOKEN to job-level env, or drops -// persist-credentials would ship without any other test to catch it. -import assert from 'node:assert/strict'; -import { readFileSync } from 'node:fs'; -import { dirname, join } from 'node:path'; -import { fileURLToPath } from 'node:url'; -import { describe, it } from 'node:test'; -import { parse } from 'yaml'; - -const workflowPath = join( - dirname(fileURLToPath(import.meta.url)), - '..', - 'workflows', - 'auto-minimize-spam.yml', -); -const doc = parse(readFileSync(workflowPath, 'utf8')); -const minimizeJob = doc.jobs.minimize; -const steps = minimizeJob.steps; -const checkoutStep = steps.find((s) => s.uses?.startsWith('actions/checkout')); -const minimizeStep = steps.find((s) => - s.name?.includes('Minimize comments'), -); - -describe('auto-minimize-spam: repository guard', () => { - it('gates the job on the canonical repository', () => { - assert.match( - String(minimizeJob.if), - /github\.repository == 'QwenLM\/qwen-code'/, - ); - }); -}); - -describe('auto-minimize-spam: permissions', () => { - it('has a minimal top-level permissions block', () => { - const perms = doc.permissions; - assert.deepEqual(perms, { - contents: 'read', - issues: 'write', - 'pull-requests': 'write', - }); - }); - - it('does not set job-level permissions', () => { - assert.equal( - minimizeJob.permissions, - undefined, - 'job-level permissions override the top-level block', - ); - }); -}); - -describe('auto-minimize-spam: credential scoping', () => { - it('disables persist-credentials on checkout', () => { - assert.ok(checkoutStep, 'checkout step must exist'); - assert.equal(checkoutStep.with['persist-credentials'], false); - }); - - it('scopes GH_TOKEN to step-level env, not job-level', () => { - assert.equal( - minimizeJob.env, - undefined, - 'job-level env would expose GH_TOKEN to every step', - ); - assert.ok(minimizeStep, 'minimize step must exist'); - assert.ok( - minimizeStep.env?.GH_TOKEN, - 'GH_TOKEN must be set in the minimize step env', - ); - }); -}); diff --git a/.github/scripts/spam-blocklist-enforce.test.mjs b/.github/scripts/spam-blocklist-enforce.test.mjs new file mode 100644 index 00000000000..70416787d90 --- /dev/null +++ b/.github/scripts/spam-blocklist-enforce.test.mjs @@ -0,0 +1,1964 @@ +// Guards for the spam-blocklist-enforce workflow. +// +// Two halves. The first follows the pattern established by +// qwen-triage-workflow.test.mjs: static assertions on the YAML, so that a +// future edit removing the repository guard, widening permissions, un-pinning +// the checkout ref or dropping persist-credentials cannot ship unnoticed. +// +// The second executes the scripts embedded in the workflow the way +// actions/github-script does — an async function over (require, github, +// context, core) — against a fake Octokit. That half exists because this +// workflow's predecessor (auto-minimize-spam) shipped a mutation its token +// could not perform: every run failed with INSUFFICIENT_SCOPES for as long as +// the blocklist was non-empty, and no static assertion would ever have caught +// it. It has already earned its keep once: it caught an early-return that +// swallowed the setFailed when the only attempted action was the one that +// failed, which would have reported a permission failure as a green run. +import assert from 'node:assert/strict'; +import { mkdtempSync, readFileSync, writeFileSync } from 'node:fs'; +import { tmpdir } from 'node:os'; +import { createRequire } from 'node:module'; +import { dirname, join } from 'node:path'; +import { fileURLToPath } from 'node:url'; +import { describe, it } from 'node:test'; +import { parse } from 'yaml'; + +const here = dirname(fileURLToPath(import.meta.url)); +const workflowPath = join( + here, + '..', + 'workflows', + 'spam-blocklist-enforce.yml', +); +const source = readFileSync(workflowPath, 'utf8'); +const doc = parse(source); + +// Every static guard below iterates all jobs, so a job added to the workflow +// later is caught by them instead of silently escaping. +const jobs = Object.entries(doc.jobs); +const scriptStepOf = (job) => + job.steps.find((step) => step.uses?.startsWith('actions/github-script')); +const checkoutStepOf = (job) => + job.steps.find((step) => step.uses?.startsWith('actions/checkout')); + +describe('spam-blocklist-enforce: step layout', () => { + for (const [name, job] of jobs) { + it(`has exactly one checkout and one github-script step in ${name}`, () => { + // A step inserted between the two would survive the one-per-kind + // assertions below and could tamper with the checked-out blocklist + // before the script reads it. + assert.equal( + job.steps.length, + 2, + 'an extra step between checkout and script can tamper with the blocklist', + ); + // Every static guard and the behavioural extraction bind to find()'s + // first match; a second checkout could shadow the blocklist with + // fork-controlled content under the write token. + assert.equal( + job.steps.filter((s) => s.uses?.startsWith('actions/checkout')).length, + 1, + ); + assert.equal( + job.steps.filter((s) => s.uses?.startsWith('actions/github-script')) + .length, + 1, + ); + }); + + it(`checks out the blocklist before the ${name} script runs`, () => { + // Reordered, the script reads a file that does not exist yet; the + // catch treats that as "no blocklist" and the lane silently no-ops. + assert.ok( + job.steps.indexOf(checkoutStepOf(job)) < + job.steps.indexOf(scriptStepOf(job)), + ); + }); + } +}); + +describe('spam-blocklist-enforce: repository guard', () => { + for (const [name, job] of jobs) { + it(`gates ${name} on the canonical repository`, () => { + assert.match(String(job.if), /github\.repository == 'QwenLM\/qwen-code'/); + }); + } + + it('routes each event to exactly one lane', () => { + // Both lanes sit under the same `on:`, so an overlap would double-act on + // one comment and a gap would silently drop an event class. Pin the full + // expressions — a substring match would still pass a mutation that + // appends another clause or weakens the repository guard. + assert.equal( + doc.jobs.enforce.if, + "${{ github.repository == 'QwenLM/qwen-code' && github.event_name != 'schedule' && github.event_name != 'workflow_dispatch' }}", + ); + assert.equal( + doc.jobs.sweep.if, + "${{ github.repository == 'QwenLM/qwen-code' && (github.event_name == 'schedule' || github.event_name == 'workflow_dispatch') }}", + ); + }); +}); + +describe('spam-blocklist-enforce: permissions', () => { + it('has a minimal top-level permissions block', () => { + assert.deepEqual(doc.permissions, { + contents: 'read', + issues: 'write', + 'pull-requests': 'write', + }); + }); + + for (const [name, job] of jobs) { + it(`does not set job-level permissions on ${name}`, () => { + assert.equal( + job.permissions, + undefined, + 'job-level permissions override the top-level block', + ); + }); + } +}); + +describe('spam-blocklist-enforce: credential scoping', () => { + for (const [name, job] of jobs) { + it(`disables persist-credentials on the ${name} checkout`, () => { + const checkout = checkoutStepOf(job); + assert.ok(checkout, 'checkout step must exist'); + assert.equal(checkout.with['persist-credentials'], false); + }); + + it(`pins the ${name} checkout to the default branch`, () => { + // pull_request_target runs with a write token. Reading the blocklist + // from anything but the default branch would let a pull request decide + // who counts as spam. + const checkout = checkoutStepOf(job); + assert.equal( + checkout.with.ref, + '${{ github.event.repository.default_branch }}', + ); + // The ref pin alone is not enough: a `repository:` input pointing at a + // fork fetches the fork's default branch — almost always named the same + // as the base's — and the fork owner decides who counts as spam. + assert.equal( + checkout.with.repository, + undefined, + 'checkout must not take a repository input', + ); + assert.equal( + checkout.with['sparse-checkout'], + '.github/spam-blocklist.txt', + ); + assert.equal( + checkout.with.path, + undefined, + 'a path input relocates the blocklist away from BLOCKLIST_PATH', + ); + }); + + it(`pins the ${name} action versions by full SHA`, () => { + // The step finders match by action-name prefix, so this is the only + // guard against a SHA downgraded to a mutable tag — one the upstream + // owner can repoint, in jobs that run on the write-scope token. + assert.equal( + checkoutStepOf(job).uses, + 'actions/checkout@df4cb1c069e1874edd31b4311f1884172cec0e10', + ); + assert.equal( + scriptStepOf(job).uses, + 'actions/github-script@3a2844b7e9c422d3c10d287c895573f7108da1b3', + ); + }); + + it(`scopes the token to the ${name} script step, not job-level env`, () => { + assert.equal( + job.env, + undefined, + 'job-level env would expose the token to every step', + ); + assert.equal( + doc.env, + undefined, + 'workflow-level env would expose secrets to every step', + ); + const step = scriptStepOf(job); + assert.ok(step, 'github-script step must exist'); + assert.equal(step.with['github-token'], '${{ secrets.GITHUB_TOKEN }}'); + }); + } + + it('never reaches for a PAT', () => { + // Deletion and review-body minimization both run on GITHUB_TOKEN under + // the permissions block above. CI_BOT_PAT must not reappear: the + // predecessor's PAT was scoped to `public_repo` only, which is exactly + // why its minimizeComment calls failed with INSUFFICIENT_SCOPES. + assert.doesNotMatch(source, /secrets\.CI_BOT_PAT/); + }); +}); + +describe('spam-blocklist-enforce: script wiring', () => { + it('wires the blocklist path into both script steps', () => { + // The step-level env is the only production connection between the YAML + // and the scripts' BLOCKLIST_PATH reads. A rename here turns the whole + // workflow into a silent no-op: readFileSync(undefined) throws, and the + // catch treats that as a missing blocklist. + for (const [name, job] of jobs) { + assert.equal( + scriptStepOf(job).env.BLOCKLIST_PATH, + '.github/spam-blocklist.txt', + `the ${name} step env must point at the checked-in blocklist`, + ); + } + }); + + it('wires the dispatch hours input into the sweep lookback', () => { + assert.equal( + scriptStepOf(doc.jobs.sweep).env.LOOKBACK_HOURS, + "${{ inputs.hours || '2' }}", + ); + assert.equal( + doc.on.workflow_dispatch?.inputs?.hours?.type, + 'number', + 'the LOOKBACK_HOURS consumer depends on this declared input', + ); + }); + + it('keeps ${{ }} expressions out of the script bodies', () => { + // The runner substitutes ${{ }} before the script text is parsed as + // JavaScript: an expression inside a script body interpolates + // event-controlled text (a comment body) into code running on the + // write token. The behavioural half cannot catch one — interpolated + // text inside a JS string literal is inert in the AsyncFunction + // harness but live in production. + for (const [name, job] of jobs) { + assert.ok( + !String(scriptStepOf(job).with.script).includes('${{'), + `expression interpolation inside the ${name} script body is code injection`, + ); + } + }); +}); + +describe('spam-blocklist-enforce: event coverage', () => { + it('listens on the comment, review, and pull request surfaces', () => { + // Known gap: commit comments are covered by neither lane — there is no + // commit_comment trigger here and the sweep does not list them. The + // predecessor did not cover them either; this pins the surfaces the + // lanes actually handle. + assert.deepEqual(Object.keys(doc.on).sort(), [ + 'issue_comment', + 'pull_request_review', + 'pull_request_review_comment', + 'pull_request_target', + 'schedule', + 'workflow_dispatch', + ]); + }); + + it('leaves the issues event to qwen-triage', () => { + // scripts/tests/issue-triage-ownership-workflow.test.js holds qwen-triage + // to being the single immediate owner of issue opened/reopened/edited. + // Adding `issues:` here would break that invariant, and would do it from a + // test file that gives no hint as to which workflow moved. Spam issues are + // closed by the sweep lane instead, within the hour. + assert.equal(doc.on.issues, undefined); + }); + + it('uses pull_request_target so fork PRs are closable', () => { + // `pull_request` hands a read-only token to fork PRs, which is exactly + // the case that needs closing. + assert.equal(doc.on.pull_request, undefined); + assert.deepEqual(doc.on.pull_request_target.types, ['opened', 'reopened']); + }); + + it('fires on edits and dismissals, not just new content', () => { + // `edited` reaches spam posted before its author was blocklisted; for + // review bodies it is the only automated path, since the sweep cannot + // list reviews. `dismissed` adds the maintainer-initiated path for the + // same class of pre-blocklist review bodies. + assert.deepEqual(doc.on.issue_comment.types, ['created', 'edited']); + assert.deepEqual(doc.on.pull_request_review_comment.types, [ + 'created', + 'edited', + ]); + assert.deepEqual(doc.on.pull_request_review.types, [ + 'submitted', + 'edited', + 'dismissed', + ]); + }); + + it('keeps an hourly scheduled backstop', () => { + // The "within the hour" promise depends on this exact cadence; any + // period longer than the 2h default lookback would make the sweep's + // blind spot permanent, not merely slower. + assert.deepEqual(doc.on.schedule, [{ cron: '30 * * * *' }]); + }); +}); + +describe('spam-blocklist-enforce: concurrency', () => { + for (const [name, job] of jobs) { + it(`never cancels an in-flight ${name} run`, () => { + // A cancelled run leaves the spam standing until the hourly sweep. + assert.equal(job.concurrency?.['cancel-in-progress'], false); + }); + } + + it('keys the enforce group on the event subject', () => { + // A comment plus its own edit must serialise on one group. Whitespace + // is normalised because the folded YAML scalar keeps literal newlines + // inside the ${{ }} expression. + assert.equal( + doc.jobs.enforce.concurrency.group.replace(/\s+/g, ' '), + 'spam-blocklist-enforce-${{ github.event.comment.id || github.event.review.id || github.event.issue.number || github.event.pull_request.number || github.run_id }}', + ); + assert.equal(doc.jobs.sweep.concurrency.group, 'spam-blocklist-sweep'); + }); +}); + +// ── Behavioural half ────────────────────────────────────────────────────── + +const AsyncFunction = Object.getPrototypeOf(async () => {}).constructor; +const nodeRequire = createRequire(import.meta.url); + +const writeBlocklist = (contents) => { + const path = join(mkdtempSync(join(tmpdir(), 'blocklist-')), 'list.txt'); + writeFileSync(path, contents); + return path; +}; + +const BLOCKLIST = writeBlocklist('# header\n\n SpamUser \n#x\nother\n'); +const EMPTY_BLOCKLIST = writeBlocklist('# only comments\n\n'); + +class HttpError extends Error { + constructor(status, message) { + super(message); + this.status = status; + } +} + +const makeCore = () => { + const logs = { + info: [], + warning: [], + notice: [], + failed: [], + summaryLists: [], + }; + const summary = { + addHeading: () => summary, + addList: (items) => { + logs.summaryLists.push([...items]); + return summary; + }, + addTable: () => summary, + write: async () => summary, + }; + return { + logs, + info: (m) => logs.info.push(m), + warning: (m) => logs.warning.push(m), + notice: (m) => logs.notice.push(m), + setFailed: (m) => logs.failed.push(m), + summary, + }; +}; + +// The fake Octokit records every mutation and returns canned pages for the +// three repo-wide listings the sweep paginates over, plus canned `data` +// replies for the read-backs and PR lookups (issues.get, pulls.get). A +// reply may also be a function of the call params, so one run can model +// per-thread verdicts. +const makeGithub = ({ calls, fail = () => null, pages = {}, replies = {} }) => { + const record = (name) => async (params) => { + calls.push({ name, params }); + const error = fail(name, params); + if (error) throw error; + const reply = replies[name]; + return { + data: typeof reply === 'function' ? reply(params) : (reply ?? {}), + }; + }; + return { + rest: { + issues: { + deleteComment: record('issues.deleteComment'), + update: record('issues.update'), + lock: record('issues.lock'), + get: record('issues.get'), + // github.paginate is handed the endpoint function itself; using the + // name as a token keeps the fake's dispatch trivial. + listCommentsForRepo: 'issues.listCommentsForRepo', + listForRepo: 'issues.listForRepo', + }, + pulls: { + deleteReviewComment: record('pulls.deleteReviewComment'), + update: record('pulls.update'), + get: record('pulls.get'), + listReviewCommentsForRepo: 'pulls.listReviewCommentsForRepo', + }, + }, + graphql: async (query, variables) => { + calls.push({ + name: 'graphql.minimizeComment', + params: variables, + query, + }); + const error = fail('graphql.minimizeComment', variables); + if (error) throw error; + return {}; + }, + paginate: async (endpoint, params) => { + const name = `paginate:${endpoint}`; + calls.push({ name, params }); + const error = fail(name, params); + if (error) throw error; + return pages[endpoint] ?? []; + }, + }; +}; + +const runLane = async (lane, { eventName, payload, env, github, core }) => { + const script = scriptStepOf(doc.jobs[lane]).with.script; + const fn = new AsyncFunction( + 'require', + 'github', + 'context', + 'core', + 'process', + script, + ); + await fn( + nodeRequire, + github, + { eventName, payload, repo: { owner: 'QwenLM', repo: 'qwen-code' } }, + core, + { env: { ...process.env, ...env } }, + ); +}; + +const names = (calls) => calls.map((call) => call.name); +const mutationsOf = (calls) => + calls.filter((call) => !call.name.startsWith('paginate:')); +const summaryItems = (core) => core.logs.summaryLists.flat(); + +const enforce = async ( + eventName, + payload, + { blocklist = BLOCKLIST, fail, replies } = {}, +) => { + const calls = []; + const core = makeCore(); + await runLane('enforce', { + eventName, + payload, + env: { BLOCKLIST_PATH: blocklist }, + github: makeGithub({ calls, fail, replies }), + core, + }); + return { calls, core }; +}; + +describe('spam-blocklist-enforce: enforce lane behaviour', () => { + it('deletes an issue comment from a blocklisted user, case-insensitively', async () => { + const { calls } = await enforce('issue_comment', { + comment: { id: 111, user: { login: 'SPAMUSER' } }, + issue: { number: 5, user: { login: 'legit' }, state: 'open' }, + }); + assert.deepEqual(names(calls), ['issues.deleteComment']); + assert.equal(calls[0].params.comment_id, 111); + }); + + it('leaves a legitimate commenter alone', async () => { + const { calls, core } = await enforce('issue_comment', { + comment: { id: 111, user: { login: 'legit' } }, + issue: { number: 5, user: { login: 'legit' }, state: 'open' }, + }); + assert.deepEqual(names(calls), []); + assert.ok(core.logs.info.some((m) => /No actions taken/.test(m))); + }); + + it('does not close an innocent PR that merely received spam', async () => { + // The whole reason closing keys on thread authorship: one spam comment + // must not close an unrelated contributor's pull request. + const { calls } = await enforce( + 'issue_comment', + { + comment: { id: 9, user: { login: 'spamuser' } }, + issue: { + number: 8626, + user: { login: 'legit' }, + state: 'open', + pull_request: {}, + }, + }, + { + replies: { + 'pulls.get': { head: { repo: { full_name: 'QwenLM/qwen-code' } } }, + }, + }, + ); + assert.deepEqual(names(calls), ['pulls.get', 'issues.deleteComment']); + }); + + it('closes and locks the thread when the blocklisted user authored it', async () => { + const { calls } = await enforce('issue_comment', { + comment: { id: 9, user: { login: 'spamuser' } }, + // Mixed-case thread author: pins case-insensitivity on the close path. + issue: { number: 42, user: { login: 'SpAmUsEr' }, state: 'open' }, + }); + assert.deepEqual(names(calls), [ + 'issues.deleteComment', + 'issues.update', + 'issues.lock', + ]); + assert.equal(calls[1].params.issue_number, 42); + assert.equal(calls[1].params.state, 'closed'); + assert.equal(calls[1].params.state_reason, 'not_planned'); + assert.equal(calls[2].params.issue_number, 42); + assert.equal(calls[2].params.lock_reason, 'spam'); + }); + + it('closes a blocklisted thread even for a legitimate comment', async () => { + // Closing keys on the thread author, not the commenter: a clean reply + // on a spam thread still closes the thread. The reply itself is not + // deleted — its author is not blocklisted. + const { calls } = await enforce('issue_comment', { + comment: { id: 9, user: { login: 'legit' } }, + issue: { number: 42, user: { login: 'spamuser' }, state: 'open' }, + }); + assert.deepEqual(names(calls), ['issues.update', 'issues.lock']); + assert.equal(calls[0].params.issue_number, 42); + assert.equal(calls[1].params.issue_number, 42); + }); + + it('routes a blocklisted author bumping their own PR through pulls.update', async () => { + const { calls } = await enforce( + 'issue_comment', + { + comment: { id: 9, user: { login: 'spamuser' } }, + issue: { + number: 77, + user: { login: 'spamuser' }, + state: 'open', + pull_request: { url: 'x' }, + }, + }, + { + replies: { + 'pulls.get': { head: { repo: { full_name: 'QwenLM/qwen-code' } } }, + }, + }, + ); + assert.deepEqual(names(calls), [ + 'pulls.get', + 'issues.deleteComment', + 'pulls.update', + 'issues.lock', + ]); + assert.equal(calls[0].params.pull_number, 77); + assert.equal(calls[2].params.pull_number, 77); + assert.equal(calls[2].params.state, 'closed'); + assert.equal(calls[3].params.issue_number, 77); + }); + + it('closes a blocklisted PR via pulls.update when the comment is legitimate', async () => { + // Closing keys on the thread author, and the same-repo head must pass + // the fork guard: a clean reply on a spammer's PR still closes the PR. + const { calls } = await enforce( + 'issue_comment', + { + comment: { id: 9, user: { login: 'legit' } }, + issue: { + number: 77, + user: { login: 'spamuser' }, + state: 'open', + pull_request: { url: 'x' }, + }, + }, + { + replies: { + 'pulls.get': { head: { repo: { full_name: 'QwenLM/qwen-code' } } }, + }, + }, + ); + assert.deepEqual(names(calls), [ + 'pulls.get', + 'pulls.update', + 'issues.lock', + ]); + assert.equal(calls[1].params.pull_number, 77); + assert.equal(calls[2].params.issue_number, 77); + }); + + it('skips a fork PR issue comment: the read-only token cannot delete it', async () => { + // Issue comments on fork PRs run on a read-only GITHUB_TOKEN just like + // review events; the write would 403 and red-run the lane. The sweep + // lane holds the write token and repairs within the hour. The payload + // carries no top-level pull_request, so the guard resolves the head + // repo through pulls.get. + const { calls, core } = await enforce( + 'issue_comment', + { + comment: { id: 111, user: { login: 'spamuser' } }, + issue: { + number: 5, + user: { login: 'legit' }, + state: 'open', + pull_request: { url: 'x' }, + }, + }, + { + replies: { + 'pulls.get': { head: { repo: { full_name: 'forker/qwen-code' } } }, + }, + }, + ); + assert.deepEqual(names(calls), ['pulls.get']); + assert.equal(calls[0].params.pull_number, 5); + assert.deepEqual(core.logs.failed, []); + assert.ok(core.logs.notice.some((m) => /fork PR/.test(m))); + }); + + it('skips a deleted-fork PR issue comment: head.repo is null', async () => { + // The deleted-fork twin: pulls.get reads head.repo: null back, which + // carries the same read-only downgrade, so the guard must fire exactly + // as for a live fork. + const { calls, core } = await enforce( + 'issue_comment', + { + comment: { id: 111, user: { login: 'spamuser' } }, + issue: { + number: 5, + user: { login: 'spamuser' }, + state: 'open', + pull_request: { url: 'x' }, + }, + }, + { replies: { 'pulls.get': { head: { repo: null } } } }, + ); + assert.deepEqual(names(calls), ['pulls.get']); + assert.deepEqual(core.logs.failed, []); + assert.ok(core.logs.notice.some((m) => /fork PR/.test(m))); + }); + + it('skips a fork PR issue comment when only the PR author is blocklisted', async () => { + // The thread-author disjunct alone: a legitimate comment on a + // blocklisted author's fork PR still runs on the read-only token, so + // the guard must fire without any comment-author involvement. + const { calls, core } = await enforce( + 'issue_comment', + { + comment: { id: 111, user: { login: 'legit' } }, + issue: { + number: 5, + user: { login: 'spamuser' }, + state: 'open', + pull_request: { url: 'x' }, + }, + }, + { + replies: { + 'pulls.get': { head: { repo: { full_name: 'forker/qwen-code' } } }, + }, + }, + ); + assert.deepEqual(names(calls), ['pulls.get']); + assert.deepEqual(core.logs.failed, []); + assert.ok(core.logs.notice.some((m) => /fork PR/.test(m))); + }); + + it('locks a closed thread whose lock failed before', async () => { + // The retry half of the lock backstop: a thread an earlier run closed + // but failed to lock still gets the lock; only the close is skipped. + const { calls } = await enforce('issue_comment', { + comment: { id: 9, user: { login: 'spamuser' } }, + issue: { number: 42, user: { login: 'spamuser' }, state: 'closed' }, + }); + assert.deepEqual(names(calls), ['issues.deleteComment', 'issues.lock']); + assert.equal(calls[1].params.issue_number, 42); + }); + + it('leaves a closed and locked thread alone', async () => { + const { calls } = await enforce('issue_comment', { + comment: { id: 9, user: { login: 'spamuser' } }, + issue: { + number: 42, + user: { login: 'spamuser' }, + state: 'closed', + locked: true, + }, + }); + assert.deepEqual(names(calls), ['issues.deleteComment']); + }); + + it('closes a locked thread whose close failed before', async () => { + // The mirror half of the leftover repair: a thread an earlier run + // locked but failed to close — or one its author reopened after the + // lock — still gets the close instead of being skipped as locked. + const { calls } = await enforce('issue_comment', { + comment: { id: 9, user: { login: 'spamuser' } }, + issue: { + number: 42, + user: { login: 'spamuser' }, + state: 'open', + locked: true, + }, + }); + assert.deepEqual(names(calls), [ + 'issues.deleteComment', + 'issues.update', + 'issues.lock', + ]); + assert.equal(calls[1].params.issue_number, 42); + assert.equal(calls[1].params.state, 'closed'); + assert.equal(calls[2].params.issue_number, 42); + }); + + it('deletes an inline review comment', async () => { + const { calls } = await enforce('pull_request_review_comment', { + comment: { id: 3734123582, user: { login: 'spamuser' } }, + }); + assert.deepEqual(names(calls), ['pulls.deleteReviewComment']); + assert.equal(calls[0].params.comment_id, 3734123582); + }); + + it('leaves a legitimate review comment author alone', async () => { + const { calls } = await enforce('pull_request_review_comment', { + comment: { id: 5, user: { login: 'legit' } }, + pull_request: { number: 60, user: { login: 'legit' }, state: 'open' }, + }); + assert.deepEqual(names(calls), []); + }); + + it('does not close a legitimate PR that merely received a blocklisted review comment', async () => { + // Closing keys on the PR author: a blocklisted reviewer must not close + // a legitimate contributor's PR — only their comment goes. + const { calls } = await enforce('pull_request_review_comment', { + comment: { id: 5, user: { login: 'spamuser' } }, + pull_request: { + number: 60, + user: { login: 'legit' }, + state: 'open', + head: { repo: { full_name: 'QwenLM/qwen-code' } }, + }, + }); + assert.deepEqual(names(calls), ['pulls.deleteReviewComment']); + }); + + it('closes the PR of a blocklisted author on a legitimate review comment', async () => { + const { calls } = await enforce('pull_request_review_comment', { + comment: { id: 5, user: { login: 'legit' } }, + pull_request: { + number: 60, + user: { login: 'spamuser' }, + state: 'open', + head: { repo: { full_name: 'QwenLM/qwen-code' } }, + }, + }); + assert.deepEqual(names(calls), ['pulls.update', 'issues.lock']); + assert.equal(calls[0].params.pull_number, 60); + assert.equal(calls[1].params.issue_number, 60); + }); + + it('deletes the comment and closes the PR when both authors are blocklisted', async () => { + // Both halves must fire together: an if/else restructuring must not + // suppress the close when the comment author is the blocklisted one. + const { calls } = await enforce('pull_request_review_comment', { + comment: { id: 5, user: { login: 'spamuser' } }, + pull_request: { + number: 60, + // Mixed-case PR author: pins case-insensitivity on the close path. + user: { login: 'SpAmUsEr' }, + state: 'open', + head: { repo: { full_name: 'QwenLM/qwen-code' } }, + }, + }); + assert.deepEqual(names(calls), [ + 'pulls.deleteReviewComment', + 'pulls.update', + 'issues.lock', + ]); + assert.equal(calls[1].params.pull_number, 60); + assert.equal(calls[2].params.issue_number, 60); + }); + + it('deletes a blocklisted review comment even on a closed and locked PR', async () => { + // Content removal is independent of the thread-state skip guard: only + // the close+lock half is skipped on a finished thread. + const { calls } = await enforce('pull_request_review_comment', { + comment: { id: 5, user: { login: 'spamuser' } }, + pull_request: { + number: 60, + user: { login: 'spamuser' }, + state: 'closed', + locked: true, + head: { repo: { full_name: 'QwenLM/qwen-code' } }, + }, + }); + assert.deepEqual(names(calls), ['pulls.deleteReviewComment']); + }); + + it('closes a locked PR whose close failed before on a review comment event', async () => { + // The mirror leftover: locked-but-open still gets its close. + const { calls } = await enforce('pull_request_review_comment', { + comment: { id: 5, user: { login: 'spamuser' } }, + pull_request: { + number: 60, + user: { login: 'spamuser' }, + state: 'open', + locked: true, + head: { repo: { full_name: 'QwenLM/qwen-code' } }, + }, + }); + assert.deepEqual(names(calls), [ + 'pulls.deleteReviewComment', + 'pulls.update', + 'issues.lock', + ]); + }); + + it('skips a fork PR review comment: the read-only token cannot delete it', async () => { + // Review events on fork PRs run on a read-only GITHUB_TOKEN; the write + // would 403 and red-run the lane. The sweep lane holds the write token + // and repairs within the hour. + const { calls, core } = await enforce('pull_request_review_comment', { + comment: { id: 5, user: { login: 'spamuser' } }, + pull_request: { + number: 60, + user: { login: 'spamuser' }, + state: 'open', + head: { repo: { full_name: 'forker/qwen-code' } }, + }, + }); + assert.deepEqual(names(calls), []); + assert.deepEqual(core.logs.failed, []); + assert.ok(core.logs.notice.some((m) => /fork PR/.test(m))); + }); + + it('still acts when the review comment head repo is the base repo', async () => { + // The fork check compares repo names: a present head repo is not a fork + // by itself. + const { calls } = await enforce('pull_request_review_comment', { + comment: { id: 5, user: { login: 'spamuser' } }, + pull_request: { + number: 60, + user: { login: 'legit' }, + state: 'open', + head: { repo: { full_name: 'QwenLM/qwen-code' } }, + }, + }); + assert.deepEqual(names(calls), ['pulls.deleteReviewComment']); + }); + + it('skips a deleted-fork PR review comment: head.repo is null', async () => { + // GitHub represents a deleted fork as head.repo: null. The token is + // still the fork-downgraded read-only one, so the guard must fire + // exactly as for a live fork instead of red-running on a 403. + const { calls, core } = await enforce('pull_request_review_comment', { + comment: { id: 5, user: { login: 'spamuser' } }, + pull_request: { + number: 60, + user: { login: 'spamuser' }, + state: 'open', + head: { repo: null }, + }, + }); + assert.deepEqual(names(calls), []); + assert.deepEqual(core.logs.failed, []); + assert.ok(core.logs.notice.some((m) => /fork PR/.test(m))); + }); + + it('skips a fork PR review comment when only the PR author is blocklisted', async () => { + // The thread-author disjunct alone: a legitimate review comment on a + // blocklisted author's fork PR hits the same read-only token and must + // defer without any comment-author involvement. + const { calls, core } = await enforce('pull_request_review_comment', { + comment: { id: 5, user: { login: 'legit' } }, + pull_request: { + number: 60, + user: { login: 'spamuser' }, + state: 'open', + head: { repo: { full_name: 'forker/qwen-code' } }, + }, + }); + assert.deepEqual(names(calls), []); + assert.deepEqual(core.logs.failed, []); + assert.ok(core.logs.notice.some((m) => /fork PR/.test(m))); + }); + + it('minimizes a review body, which has no REST delete', async () => { + const { calls } = await enforce('pull_request_review', { + review: { id: 7, node_id: 'PRR_abc', user: { login: 'spamuser' } }, + }); + assert.deepEqual(names(calls), ['graphql.minimizeComment']); + assert.equal(calls[0].params.id, 'PRR_abc'); + assert.match(calls[0].query, /minimizeComment/); + assert.match(calls[0].query, /classifier: SPAM/); + }); + + it('leaves a legitimate review author alone', async () => { + const { calls } = await enforce('pull_request_review', { + review: { id: 6, node_id: 'PRR_ok', user: { login: 'legit' } }, + pull_request: { number: 61, user: { login: 'legit' }, state: 'open' }, + }); + assert.deepEqual(names(calls), []); + }); + + it('closes the PR of a blocklisted author on a legitimate review', async () => { + const { calls } = await enforce('pull_request_review', { + review: { id: 6, node_id: 'PRR_ok', user: { login: 'legit' } }, + pull_request: { + number: 61, + user: { login: 'spamuser' }, + state: 'open', + head: { repo: { full_name: 'QwenLM/qwen-code' } }, + }, + }); + assert.deepEqual(names(calls), ['pulls.update', 'issues.lock']); + assert.equal(calls[0].params.pull_number, 61); + assert.equal(calls[1].params.issue_number, 61); + }); + + it('minimizes the review and closes the PR when both authors are blocklisted', async () => { + const { calls } = await enforce('pull_request_review', { + review: { id: 7, node_id: 'PRR_spam', user: { login: 'spamuser' } }, + pull_request: { + number: 61, + // Mixed-case PR author: pins case-insensitivity on the close path. + user: { login: 'SpAmUsEr' }, + state: 'open', + head: { repo: { full_name: 'QwenLM/qwen-code' } }, + }, + }); + assert.deepEqual(names(calls), [ + 'graphql.minimizeComment', + 'pulls.update', + 'issues.lock', + ]); + assert.equal(calls[1].params.pull_number, 61); + assert.equal(calls[2].params.issue_number, 61); + }); + + it('does not close a legitimate PR that merely received a blocklisted review', async () => { + // The R6-3 twin for this lane: only the review body is minimized, the + // legitimate author's PR stays open. + const { calls } = await enforce('pull_request_review', { + review: { id: 7, node_id: 'PRR_spam', user: { login: 'spamuser' } }, + pull_request: { + number: 61, + user: { login: 'legit' }, + state: 'open', + head: { repo: { full_name: 'QwenLM/qwen-code' } }, + }, + }); + assert.deepEqual(names(calls), ['graphql.minimizeComment']); + }); + + it('minimizes a blocklisted review even on a closed and locked PR', async () => { + // Content removal is independent of the thread-state skip guard; a + // later dismissal of this review must still reach the minimize even + // though the thread is finished. + const { calls } = await enforce('pull_request_review', { + review: { id: 7, node_id: 'PRR_abc', user: { login: 'spamuser' } }, + pull_request: { + number: 61, + user: { login: 'spamuser' }, + state: 'closed', + locked: true, + head: { repo: { full_name: 'QwenLM/qwen-code' } }, + }, + }); + assert.deepEqual(names(calls), ['graphql.minimizeComment']); + }); + + it('closes a locked PR whose close failed before on a review event', async () => { + // The mirror leftover: locked-but-open still gets its close. + const { calls } = await enforce('pull_request_review', { + review: { id: 6, node_id: 'PRR_ok', user: { login: 'legit' } }, + pull_request: { + number: 61, + user: { login: 'spamuser' }, + state: 'open', + locked: true, + head: { repo: { full_name: 'QwenLM/qwen-code' } }, + }, + }); + assert.deepEqual(names(calls), ['pulls.update', 'issues.lock']); + }); + + it('skips a fork PR review body: the read-only token cannot minimize it', async () => { + // No sweep backstop exists for review bodies, so the notice is the + // whole automated response: the body needs manual minimization. + const { calls, core } = await enforce('pull_request_review', { + review: { id: 7, node_id: 'PRR_spam', user: { login: 'spamuser' } }, + pull_request: { + number: 61, + user: { login: 'spamuser' }, + state: 'open', + head: { repo: { full_name: 'forker/qwen-code' } }, + }, + }); + assert.deepEqual(names(calls), []); + assert.deepEqual(core.logs.failed, []); + assert.ok(core.logs.notice.some((m) => /manual minimization/.test(m))); + }); + + it('skips a deleted-fork PR review body: head.repo is null', async () => { + // The deleted-fork twin: head.repo: null must fire the guard and leave + // the manual-minimization notice behind, because no lane can minimize + // a review body on a read-only token. + const { calls, core } = await enforce('pull_request_review', { + review: { id: 7, node_id: 'PRR_spam', user: { login: 'spamuser' } }, + pull_request: { + number: 61, + user: { login: 'spamuser' }, + state: 'open', + head: { repo: null }, + }, + }); + assert.deepEqual(names(calls), []); + assert.deepEqual(core.logs.failed, []); + assert.ok(core.logs.notice.some((m) => /manual minimization/.test(m))); + }); + + it('skips a fork PR review body when only the PR author is blocklisted', async () => { + // The thread-author disjunct alone: a legitimate review on a + // blocklisted author's fork PR cannot be minimized on the read-only + // token, so the manual-minimization notice must fire without any + // review-author involvement. + const { calls, core } = await enforce('pull_request_review', { + review: { id: 6, node_id: 'PRR_ok', user: { login: 'legit' } }, + pull_request: { + number: 61, + user: { login: 'spamuser' }, + state: 'open', + head: { repo: { full_name: 'forker/qwen-code' } }, + }, + }); + assert.deepEqual(names(calls), []); + assert.deepEqual(core.logs.failed, []); + assert.ok(core.logs.notice.some((m) => /manual minimization/.test(m))); + }); + + it('ignores an issues event, which this workflow no longer subscribes to', async () => { + // Pins the current behaviour: the script has no `issues` branch, so an + // issues event is a no-op and spam issues wait for the sweep lane. The + // `on:` assertion above is what keeps the trigger out; if someone + // restores it, that assertion sends them here, to the pinned no-op — not + // to a script that silently handles issue events some other way. + const { calls, core } = await enforce('issues', { + action: 'opened', + issue: { number: 100, user: { login: 'other' } }, + }); + assert.deepEqual(names(calls), []); + assert.deepEqual(core.logs.failed, []); + }); + + it('closes a fork PR through pulls.update but locks through issues.lock', async () => { + const { calls } = await enforce('pull_request_target', { + // Mixed-case PR author: pins case-insensitivity on the close path. + // Fork head: this lane exists so fork PRs are closable — a guard + // that deferred on the fork head would silently no-op the lane. + pull_request: { + number: 101, + user: { login: 'SpAmUsEr' }, + head: { repo: { full_name: 'forker/qwen-code' } }, + }, + }); + assert.deepEqual(names(calls), ['pulls.update', 'issues.lock']); + assert.equal(calls[0].params.pull_number, 101); + assert.equal(calls[0].params.state, 'closed'); + assert.equal(calls[1].params.issue_number, 101); + }); + + it('closes a deleted-fork PR too: head.repo is null', async () => { + // A deleted fork must not escape the close through a head-presence + // guard — the lane exists exactly so fork PRs are closable. + const { calls } = await enforce('pull_request_target', { + pull_request: { + number: 103, + user: { login: 'spamuser' }, + state: 'open', + head: { repo: null }, + }, + }); + assert.deepEqual(names(calls), ['pulls.update', 'issues.lock']); + assert.equal(calls[0].params.pull_number, 103); + }); + + it('leaves a legitimate PR author alone', async () => { + const { calls } = await enforce('pull_request_target', { + pull_request: { number: 102, user: { login: 'legit' }, state: 'open' }, + }); + assert.deepEqual(names(calls), []); + }); + + it('treats a 404 as already-done rather than a failure', async () => { + const { core } = await enforce( + 'issue_comment', + { + comment: { id: 111, user: { login: 'spamuser' } }, + issue: { number: 5, user: { login: 'legit' }, state: 'open' }, + }, + { fail: () => new HttpError(404, 'Not Found') }, + ); + assert.deepEqual(core.logs.failed, []); + assert.ok(core.logs.info.some((m) => /already gone/.test(m))); + }); + + it('treats a lock 422 as already-done once the thread reads back locked', async () => { + // issues.lock answers 422 when the thread is already locked — a + // concurrent run won the race — but GitHub also answers 422 for + // validation and abuse-protection failures, so the lane reads the + // lock state back before accepting it. + const { calls, core } = await enforce( + 'issue_comment', + { + comment: { id: 9, user: { login: 'spamuser' } }, + issue: { number: 42, user: { login: 'spamuser' }, state: 'open' }, + }, + { + fail: (name) => + name === 'issues.lock' ? new HttpError(422, 'Already locked') : null, + replies: { 'issues.get': { locked: true } }, + }, + ); + assert.deepEqual(core.logs.failed, []); + const readback = calls.find((call) => call.name === 'issues.get'); + assert.equal(readback.params.issue_number, 42); + }); + + it('fails the run when a lock 422 reads back unlocked', async () => { + // The unverified half of the race: an unlocked readback means the 422 + // was a validation/abuse rejection, not a concurrent lock — the lock + // never happened and the run must fail. + const { core } = await enforce( + 'issue_comment', + { + comment: { id: 9, user: { login: 'spamuser' } }, + issue: { number: 42, user: { login: 'spamuser' }, state: 'open' }, + }, + { + fail: (name) => + name === 'issues.lock' + ? new HttpError(422, 'Validation Failed') + : null, + replies: { 'issues.get': { locked: false } }, + }, + ); + assert.equal(core.logs.failed.length, 1); + assert.match(core.logs.failed[0], /lock .*422/); + }); + + it('fails the run when the lock-race readback itself fails', async () => { + // Fail closed: if the lock state cannot be read back, the 422 stays + // unverified and counts as the lock failure it may be. + const { core } = await enforce( + 'issue_comment', + { + comment: { id: 9, user: { login: 'spamuser' } }, + issue: { number: 42, user: { login: 'spamuser' }, state: 'open' }, + }, + { + fail: (name) => + name === 'issues.lock' + ? new HttpError(422, 'Already locked') + : name === 'issues.get' + ? new HttpError(403, 'rate limited') + : null, + }, + ); + assert.equal(core.logs.failed.length, 1); + assert.match(core.logs.failed[0], /lock .*422/); + }); + + it('fails the run when the issue-comment delete 422s', async () => { + // A 422 cannot prove the comment is gone — GitHub also uses it for + // validation and abuse-protection failures — so the run must fail. + const { core } = await enforce( + 'issue_comment', + { + comment: { id: 111, user: { login: 'spamuser' } }, + issue: { number: 5, user: { login: 'legit' }, state: 'open' }, + }, + { + fail: (name) => + name === 'issues.deleteComment' + ? new HttpError(422, 'Validation Failed') + : null, + }, + ); + assert.equal(core.logs.failed.length, 1); + assert.match(core.logs.failed[0], /delete issue comment .*422/); + }); + + it('fails the run when the review-comment delete 422s', async () => { + const { core } = await enforce( + 'pull_request_review_comment', + { + comment: { id: 5, user: { login: 'spamuser' } }, + pull_request: { + number: 60, + user: { login: 'legit' }, + state: 'open', + head: { repo: { full_name: 'QwenLM/qwen-code' } }, + }, + }, + { + fail: (name) => + name === 'pulls.deleteReviewComment' + ? new HttpError(422, 'Validation Failed') + : null, + }, + ); + assert.equal(core.logs.failed.length, 1); + assert.match(core.logs.failed[0], /delete review comment .*422/); + }); + + it('fails the run when the issue close 422s', async () => { + const { core } = await enforce( + 'issue_comment', + { + comment: { id: 9, user: { login: 'legit' } }, + issue: { number: 42, user: { login: 'spamuser' }, state: 'open' }, + }, + { + fail: (name) => + name === 'issues.update' + ? new HttpError(422, 'Validation Failed') + : null, + }, + ); + assert.equal(core.logs.failed.length, 1); + assert.match(core.logs.failed[0], /close issue #42: 422/); + }); + + it('fails the run when the PR close 422s', async () => { + const { core } = await enforce( + 'pull_request_target', + { + pull_request: { number: 101, user: { login: 'spamuser' } }, + }, + { + fail: (name) => + name === 'pulls.update' + ? new HttpError(422, 'Validation Failed') + : null, + }, + ); + assert.equal(core.logs.failed.length, 1); + assert.match(core.logs.failed[0], /close pull request #101: 422/); + }); + + it('fails the run when the review-body minimize 422s', async () => { + // A review body has no sweep backstop: a failed minimize must red-run + // so the run failure itself flags the need for manual cleanup. + const { core } = await enforce( + 'pull_request_review', + { + review: { id: 7, node_id: 'PRR_spam', user: { login: 'spamuser' } }, + }, + { + fail: (name) => + name === 'graphql.minimizeComment' + ? new HttpError(422, 'Validation Failed') + : null, + }, + ); + assert.equal(core.logs.failed.length, 1); + assert.match(core.logs.failed[0], /minimize review 7: 422/); + }); + + it('keeps closing the thread after a delete 404s', async () => { + // A 404 means the comment is already gone — the lane must still close + // and lock the spammer's thread, not treat the event as all-done. + const { calls, core } = await enforce( + 'issue_comment', + { + comment: { id: 111, user: { login: 'spamuser' } }, + issue: { number: 5, user: { login: 'spamuser' }, state: 'open' }, + }, + { + fail: (name) => + name === 'issues.deleteComment' + ? new HttpError(404, 'Not Found') + : null, + }, + ); + assert.deepEqual(core.logs.failed, []); + assert.deepEqual(names(mutationsOf(calls)), [ + 'issues.deleteComment', + 'issues.update', + 'issues.lock', + ]); + }); + + it('treats a close 404 as already-done and still locks the thread', async () => { + // A thread deleted between the payload snapshot and the close attempt + // already holds the desired end state; the 404 must not red-run the + // lane, and the lock that follows the close must still be attempted. + const { calls, core } = await enforce( + 'issue_comment', + { + comment: { id: 9, user: { login: 'legit' } }, + issue: { number: 42, user: { login: 'spamuser' }, state: 'open' }, + }, + { + fail: (name) => + name === 'issues.update' ? new HttpError(404, 'Not Found') : null, + }, + ); + assert.deepEqual(core.logs.failed, []); + assert.ok(core.logs.info.some((m) => /already gone/.test(m))); + assert.deepEqual(names(mutationsOf(calls)), [ + 'issues.update', + 'issues.lock', + ]); + }); + + it('treats a PR close 404 and lock 404 as already-done', async () => { + // The PR-shape twin: a PR deleted between the event and the close + // attempt 404s on both the close and the lock that follows — both + // prove the target is gone, so the run stays green. + const { calls, core } = await enforce( + 'pull_request_target', + { + pull_request: { number: 101, user: { login: 'spamuser' } }, + }, + { + fail: (name) => + name === 'pulls.update' || name === 'issues.lock' + ? new HttpError(404, 'Not Found') + : null, + }, + ); + assert.deepEqual(core.logs.failed, []); + assert.equal( + core.logs.info.filter((m) => /already gone/.test(m)).length, + 2, + ); + assert.deepEqual(names(mutationsOf(calls)), [ + 'pulls.update', + 'issues.lock', + ]); + }); + + it('still closes the thread when the delete 403s', async () => { + // The hard-failure twin: a non-404/422 delete error (a rate-limit 403 + // is realistic — the token's limit is shared with every enforce run of + // the same hour) collects as a run failure but must not gate the close. + const { calls, core } = await enforce( + 'issue_comment', + { + comment: { id: 9, user: { login: 'spamuser' } }, + issue: { number: 42, user: { login: 'spamuser' }, state: 'open' }, + }, + { + fail: (name) => + name === 'issues.deleteComment' + ? new HttpError(403, 'Forbidden') + : null, + }, + ); + assert.equal(core.logs.failed.length, 1); + assert.match(core.logs.failed[0], /403/); + assert.deepEqual(names(mutationsOf(calls)), [ + 'issues.deleteComment', + 'issues.update', + 'issues.lock', + ]); + }); + + it('fails the run when the only attempted action errors', async () => { + // The regression this half was written for: with `actions` empty the + // early return used to fire before setFailed, turning a 403 into a green + // run — the exact way the predecessor's broken token went unnoticed. + const { core } = await enforce( + 'issue_comment', + { + comment: { id: 111, user: { login: 'spamuser' } }, + issue: { number: 5, user: { login: 'legit' }, state: 'open' }, + }, + { fail: () => new HttpError(403, 'Forbidden') }, + ); + assert.equal(core.logs.failed.length, 1); + assert.match(core.logs.failed[0], /403/); + assert.ok(summaryItems(core).some((item) => item.startsWith('FAILED — '))); + }); + + it('fails the run when some actions succeed and others do not', async () => { + const { calls, core } = await enforce( + 'issue_comment', + { + comment: { id: 9, user: { login: 'spamuser' } }, + issue: { number: 42, user: { login: 'spamuser' }, state: 'open' }, + }, + { + fail: (name) => + name === 'issues.lock' ? new HttpError(403, 'Forbidden') : null, + }, + ); + assert.deepEqual(names(mutationsOf(calls)), [ + 'issues.deleteComment', + 'issues.update', + 'issues.lock', + ]); + assert.equal(core.logs.failed.length, 1); + assert.match(core.logs.failed[0], /403/); + assert.ok(summaryItems(core).some((item) => item.startsWith('FAILED — '))); + }); + + it('still locks the thread when the close fails mid-sequence', async () => { + // A rate-limit 403 on the close must not skip the lock that follows it; + // the sweep backstop repairs leftovers, but the lane must attempt them. + const { calls, core } = await enforce( + 'issue_comment', + { + comment: { id: 9, user: { login: 'spamuser' } }, + issue: { number: 42, user: { login: 'spamuser' }, state: 'open' }, + }, + { + fail: (name) => + name === 'issues.update' ? new HttpError(403, 'Forbidden') : null, + }, + ); + const mutations = mutationsOf(calls); + assert.deepEqual(names(mutations), [ + 'issues.deleteComment', + 'issues.update', + 'issues.lock', + ]); + assert.equal(mutations[1].params.issue_number, 42); + assert.equal(mutations[2].params.issue_number, 42); + assert.equal(core.logs.failed.length, 1); + assert.match(core.logs.failed[0], /403/); + }); + + it('is a no-op on an empty blocklist', async () => { + const { calls, core } = await enforce( + 'issue_comment', + { + comment: { id: 1, user: { login: 'spamuser' } }, + issue: { number: 1, user: { login: 'x' }, state: 'open' }, + }, + { blocklist: EMPTY_BLOCKLIST }, + ); + assert.deepEqual(names(calls), []); + assert.deepEqual(core.logs.failed, []); + assert.ok(core.logs.info.some((m) => /empty/.test(m))); + }); + + it('is a no-op when the blocklist file is missing', async () => { + const { calls, core } = await enforce( + 'issue_comment', + { + comment: { id: 1, user: { login: 'spamuser' } }, + issue: { number: 1, user: { login: 'x' }, state: 'open' }, + }, + { blocklist: join(tmpdir(), 'no-such-blocklist.txt') }, + ); + assert.deepEqual(names(calls), []); + assert.deepEqual(core.logs.failed, []); + assert.ok(core.logs.info.some((m) => /No blocklist/.test(m))); + }); + + it('survives a ghost author on a deleted account', async () => { + const issueComment = await enforce('issue_comment', { + comment: { id: 1, user: null }, + issue: { number: 1, user: null, state: 'open' }, + }); + assert.deepEqual(names(issueComment.calls), []); + assert.deepEqual(issueComment.core.logs.failed, []); + + const reviewComment = await enforce('pull_request_review_comment', { + comment: { id: 1, user: null }, + pull_request: { number: 1, user: null, state: 'open' }, + }); + assert.deepEqual(names(reviewComment.calls), []); + assert.deepEqual(reviewComment.core.logs.failed, []); + + const review = await enforce('pull_request_review', { + review: { id: 1, node_id: 'PRR_ghost', user: null }, + pull_request: { number: 1, user: null, state: 'open' }, + }); + assert.deepEqual(names(review.calls), []); + assert.deepEqual(review.core.logs.failed, []); + + const openedPr = await enforce('pull_request_target', { + pull_request: { number: 1, user: null, state: 'open' }, + }); + assert.deepEqual(names(openedPr.calls), []); + assert.deepEqual(openedPr.core.logs.failed, []); + }); + + it('still closes a blocklisted PR when the reviewer is a deleted account', async () => { + // The reviewer check precedes closeThread: a ghost reviewer must not + // let a blocklisted author's PR escape the close. + const { calls } = await enforce('pull_request_review', { + review: { id: 1, node_id: 'PRR_ghost', user: null }, + pull_request: { + number: 2, + user: { login: 'spamuser' }, + state: 'open', + head: { repo: { full_name: 'QwenLM/qwen-code' } }, + }, + }); + assert.deepEqual(names(calls), ['pulls.update', 'issues.lock']); + assert.equal(calls[0].params.pull_number, 2); + assert.equal(calls[1].params.issue_number, 2); + }); +}); + +describe('spam-blocklist-enforce: sweep lane behaviour', () => { + const sweep = async ({ + issueComments = [], + reviewComments = [], + threads = [], + fail, + replies, + blocklist = BLOCKLIST, + lookbackHours, + } = {}) => { + const calls = []; + const core = makeCore(); + await runLane('sweep', { + eventName: 'schedule', + payload: {}, + env: { BLOCKLIST_PATH: blocklist, LOOKBACK_HOURS: lookbackHours }, + github: makeGithub({ + calls, + fail, + replies, + pages: { + 'issues.listCommentsForRepo': issueComments, + 'pulls.listReviewCommentsForRepo': reviewComments, + 'issues.listForRepo': threads, + }, + }), + core, + }); + return { calls, core }; + }; + + it('deletes blocklisted comments of both kinds and skips the rest', async () => { + const { calls } = await sweep({ + issueComments: [ + { id: 1, user: { login: 'legit' } }, + { id: 2, user: { login: 'SpamUser' } }, + ], + reviewComments: [ + { id: 3, user: { login: 'other' } }, + { id: 4, user: { login: 'legit' } }, + ], + }); + assert.deepEqual( + mutationsOf(calls).map((call) => [call.name, call.params.comment_id]), + [ + ['issues.deleteComment', 2], + ['pulls.deleteReviewComment', 3], + ], + ); + }); + + it('does not close a legitimate thread that merely received a blocklisted comment', async () => { + // The sweep's close predicate keys on thread authorship exactly like + // the event lanes: a blocklisted commenter on a legitimate thread only + // loses the comment. + const { calls } = await sweep({ + issueComments: [{ id: 5, user: { login: 'spamuser' } }], + threads: [{ number: 30, user: { login: 'legit' }, state: 'open' }], + }); + assert.deepEqual(names(mutationsOf(calls)), ['issues.deleteComment']); + }); + + it('closes blocklisted-authored threads, routing PRs to pulls.update', () => + sweep({ + threads: [ + { number: 10, user: { login: 'legit' }, state: 'open' }, + // Mixed-case thread author: pins case-insensitivity on the close + // path. + { number: 11, user: { login: 'SpAmUsEr' }, state: 'open' }, + { + number: 12, + user: { login: 'other' }, + pull_request: { url: 'x' }, + state: 'open', + }, + ], + }).then(({ calls }) => { + const mutations = mutationsOf(calls); + assert.deepEqual(names(mutations), [ + 'issues.update', + 'issues.lock', + 'pulls.update', + 'issues.lock', + ]); + assert.equal(mutations[0].params.issue_number, 11); + assert.equal(mutations[0].params.state, 'closed'); + assert.equal(mutations[0].params.state_reason, 'not_planned'); + assert.equal(mutations[1].params.issue_number, 11); + assert.equal(mutations[1].params.lock_reason, 'spam'); + assert.equal(mutations[2].params.pull_number, 12); + assert.equal(mutations[2].params.state, 'closed'); + assert.equal(mutations[3].params.issue_number, 12); + assert.equal(mutations[3].params.lock_reason, 'spam'); + })); + + it('locks closed-but-unlocked threads and closes locked-but-open ones', async () => { + // `locked` guards only the lock call, never the close: a thread a + // previous run closed but failed to lock must get its lock, and the + // mirror leftover — a close that failed before the lock landed — must + // still get its close on the next sweep. + const { calls } = await sweep({ + threads: [ + { + number: 20, + user: { login: 'spamuser' }, + state: 'closed', + locked: true, + }, + { number: 21, user: { login: 'spamuser' }, state: 'closed' }, + { number: 22, user: { login: 'spamuser' }, state: 'open' }, + { + number: 23, + user: { login: 'spamuser' }, + state: 'open', + locked: true, + }, + ], + }); + const mutations = mutationsOf(calls); + assert.deepEqual(names(mutations), [ + 'issues.lock', + 'issues.update', + 'issues.lock', + 'issues.update', + ]); + assert.equal(mutations[0].params.issue_number, 21); + assert.equal(mutations[1].params.issue_number, 22); + assert.equal(mutations[1].params.state, 'closed'); + assert.equal(mutations[2].params.issue_number, 22); + assert.equal(mutations[3].params.issue_number, 23); + assert.equal(mutations[3].params.state, 'closed'); + }); + + it('scopes all three listings to the lookback window', async () => { + // No lookbackHours passed: this also pins the `|| 2` default. + const { calls } = await sweep({}); + const paginated = calls.filter((call) => call.name.startsWith('paginate:')); + assert.equal(paginated.length, 3); + for (const call of paginated) { + const age = Date.now() - Date.parse(call.params.since); + assert.ok( + age > 1.9 * 3600e3 && age < 2.1 * 3600e3, + `since=${call.params.since} is not ~2h old`, + ); + } + assert.equal( + calls.find((call) => call.name === 'paginate:issues.listForRepo').params + .state, + 'all', + "'all', not 'open': closed-but-unlocked threads are what the sweep repairs", + ); + }); + + it('honours a non-default LOOKBACK_HOURS', async () => { + const { calls } = await sweep({ lookbackHours: '24' }); + const paginated = calls.filter((call) => call.name.startsWith('paginate:')); + assert.equal(paginated.length, 3); + for (const call of paginated) { + const age = Date.now() - Date.parse(call.params.since); + assert.ok( + age > 23.9 * 3600e3 && age < 24.1 * 3600e3, + `since=${call.params.since} is not ~24h old`, + ); + } + }); + + it('fails the run on a failed mutation and still takes the rest', async () => { + const { calls, core } = await sweep({ + issueComments: [{ id: 2, user: { login: 'spamuser' } }], + threads: [{ number: 11, user: { login: 'spamuser' }, state: 'open' }], + fail: (name) => + name === 'issues.deleteComment' + ? new HttpError(403, 'Forbidden') + : null, + }); + assert.equal(core.logs.failed.length, 1); + assert.match(core.logs.failed[0], /403/); + assert.deepEqual(names(mutationsOf(calls)), [ + 'issues.deleteComment', + 'issues.update', + 'issues.lock', + ]); + assert.ok(summaryItems(core).some((item) => item.startsWith('FAILED — '))); + }); + + it('treats a sweep 404 as already-done', async () => { + const { core } = await sweep({ + issueComments: [{ id: 2, user: { login: 'spamuser' } }], + fail: (name) => + name === 'issues.deleteComment' + ? new HttpError(404, 'Not Found') + : null, + }); + assert.deepEqual(core.logs.failed, []); + assert.ok(core.logs.info.some((m) => /already gone/.test(m))); + }); + + it('treats a sweep close 404 and lock 404 as already-done', async () => { + // Sweep listings are stale by construction: a thread deleted between + // listForRepo and the close attempt 404s on the close, and the lock + // on the gone thread 404s too — the desired end state already holds, + // so neither red-runs the sweep. + const { calls, core } = await sweep({ + threads: [{ number: 11, user: { login: 'spamuser' }, state: 'open' }], + fail: (name) => + name === 'issues.update' || name === 'issues.lock' + ? new HttpError(404, 'Not Found') + : null, + }); + assert.deepEqual(core.logs.failed, []); + assert.equal( + core.logs.info.filter((m) => /already gone/.test(m)).length, + 2, + ); + assert.deepEqual(names(mutationsOf(calls)), [ + 'issues.update', + 'issues.lock', + ]); + }); + + it('treats a sweep lock 422 as already-done once it reads back locked', async () => { + // A concurrent run locking the thread between listForRepo and + // issues.lock must not read as a sweep failure — but the lock state + // is read back first, because 422 also signals validation failures. + const { calls, core } = await sweep({ + threads: [{ number: 11, user: { login: 'spamuser' }, state: 'open' }], + fail: (name) => + name === 'issues.lock' ? new HttpError(422, 'Already locked') : null, + replies: { 'issues.get': { locked: true } }, + }); + assert.deepEqual(core.logs.failed, []); + const readback = calls.find((call) => call.name === 'issues.get'); + assert.equal(readback.params.issue_number, 11); + }); + + it('fails the sweep when a lock 422 reads back unlocked', async () => { + const { core } = await sweep({ + threads: [{ number: 11, user: { login: 'spamuser' }, state: 'open' }], + fail: (name) => + name === 'issues.lock' ? new HttpError(422, 'Validation Failed') : null, + replies: { 'issues.get': { locked: false } }, + }); + assert.equal(core.logs.failed.length, 1); + assert.match(core.logs.failed[0], /lock .*422/); + }); + + it('keeps lock-422 verdicts per-thread across one sweep', async () => { + // Two blocklisted leftovers, both lock 422s: one reads back locked + // (a genuine race — tolerated), the other reads back unlocked (a + // validation failure — must fail). A memoized or otherwise shared + // readback would leak one thread's verdict onto the other. + const { calls, core } = await sweep({ + threads: [ + { number: 31, user: { login: 'spamuser' }, state: 'open' }, + { number: 32, user: { login: 'spamuser' }, state: 'open' }, + ], + fail: (name) => + name === 'issues.lock' ? new HttpError(422, 'Already locked') : null, + replies: { + 'issues.get': (params) => ({ locked: params.issue_number === 31 }), + }, + }); + assert.deepEqual( + names(mutationsOf(calls)).filter((name) => name !== 'issues.get'), + ['issues.update', 'issues.lock', 'issues.update', 'issues.lock'], + ); + assert.deepEqual( + calls + .filter((call) => call.name === 'issues.get') + .map((call) => call.params.issue_number), + [31, 32], + ); + assert.equal(core.logs.failed.length, 1); + assert.match(core.logs.failed[0], /lock issue #32.*422/); + }); + + it('fails the sweep when a delete 422s', async () => { + // The sweep twin of the enforce pin: a 422 cannot prove the comment + // is gone, so it must red-flag the run instead of logging it as + // already gone. + const { core } = await sweep({ + issueComments: [{ id: 2, user: { login: 'spamuser' } }], + fail: (name) => + name === 'issues.deleteComment' + ? new HttpError(422, 'Validation Failed') + : null, + }); + assert.equal(core.logs.failed.length, 1); + assert.match(core.logs.failed[0], /delete issue comment .*422/); + }); + + it('still locks a sweep thread when the close fails mid-sequence', async () => { + // Mirror of the enforce lane's pin: the close failure collects into the + // run failure, but the lock must still be attempted — the leftover + // state the sweep exists to repair. + const { calls, core } = await sweep({ + threads: [{ number: 11, user: { login: 'spamuser' }, state: 'open' }], + fail: (name) => + name === 'issues.update' ? new HttpError(403, 'Forbidden') : null, + }); + assert.equal(core.logs.failed.length, 1); + assert.match(core.logs.failed[0], /403/); + assert.deepEqual(names(mutationsOf(calls)), [ + 'issues.update', + 'issues.lock', + ]); + }); + + // The listings run through run() like everything else: a rate-limit 403 + // on any one of them must collect as a failure while the rest of the + // sweep still executes, never aborting the lane before its remaining + // mutations. Survivors differ per case: whatever the other two listings + // feed. + const listingSurvivors = { + 'paginate:issues.listCommentsForRepo': ['issues.update', 'issues.lock'], + 'paginate:pulls.listReviewCommentsForRepo': [ + 'issues.deleteComment', + 'issues.update', + 'issues.lock', + ], + 'paginate:issues.listForRepo': ['issues.deleteComment'], + }; + for (const [listing, survivors] of Object.entries(listingSurvivors)) { + it(`keeps sweeping when ${listing.replace('paginate:', '')} fails`, async () => { + const { calls, core } = await sweep({ + issueComments: [{ id: 2, user: { login: 'spamuser' } }], + threads: [{ number: 11, user: { login: 'spamuser' }, state: 'open' }], + fail: (name) => + name === listing ? new HttpError(403, 'rate limited') : null, + }); + assert.equal(core.logs.failed.length, 1); + assert.match(core.logs.failed[0], /403/); + assert.deepEqual(names(mutationsOf(calls)), survivors); + assert.ok( + summaryItems(core).some((item) => item.startsWith('FAILED — ')), + ); + }); + } + + // Neither a 404 nor a 422 can prove there was nothing to scan: any + // non-success listing response fails the run — silently scanning zero + // items is exactly how blocklisted content stays visible. + for (const listing of [ + 'paginate:issues.listCommentsForRepo', + 'paginate:pulls.listReviewCommentsForRepo', + 'paginate:issues.listForRepo', + ]) { + for (const status of [404, 422]) { + it(`fails the run when ${listing.replace('paginate:', '')} answers ${status}`, async () => { + const { calls, core } = await sweep({ + issueComments: [{ id: 2, user: { login: 'spamuser' } }], + fail: (name) => + name === listing ? new HttpError(status, 'listing failed') : null, + }); + assert.equal(core.logs.failed.length, 1); + assert.match(core.logs.failed[0], new RegExp(String(status))); + // The remaining listings still run: one failed listing must not + // abort the sweep before the other two scans. + const paginated = calls.filter((call) => + call.name.startsWith('paginate:'), + ); + assert.equal(paginated.length, 3); + }); + } + } + + it('survives deleted accounts across all three listings', async () => { + const { calls, core } = await sweep({ + issueComments: [{ id: 1, user: null }], + reviewComments: [{ id: 2, user: null }], + threads: [{ number: 3, user: null, state: 'open' }], + }); + assert.deepEqual(mutationsOf(calls), []); + assert.deepEqual(core.logs.failed, []); + }); + + it('is a no-op on an empty blocklist', async () => { + const { calls, core } = await sweep({ + blocklist: EMPTY_BLOCKLIST, + issueComments: [{ id: 1, user: { login: 'spamuser' } }], + }); + assert.deepEqual(calls, []); + assert.deepEqual(core.logs.failed, []); + assert.ok(core.logs.info.some((m) => /empty/.test(m))); + }); + + it('is a no-op when the blocklist file is missing', async () => { + const { calls, core } = await sweep({ + blocklist: join(tmpdir(), 'no-such-blocklist.txt'), + }); + assert.deepEqual(calls, []); + assert.deepEqual(core.logs.failed, []); + assert.ok(core.logs.info.some((m) => /No blocklist/.test(m))); + }); +}); + +describe('spam-blocklist-enforce: blocklist file', () => { + it('embeds the same parser and helpers in both lanes', () => { + // Compare the two definitions, not just their presence: one-sided drift + // would make the lanes disagree about who is blocklisted, whether a 422 + // lock error was a race, or what a run() call returned. + const helperOf = (job, name) => { + const script = scriptStepOf(job).with.script; + const start = script.indexOf(`const ${name} =`); + // Slice to the blank line, not the first ';': once a helper gains a + // multi-statement body, a ';' slice truncates the comparison and + // hides one-sided drift in any later statement. `-1` (a helper that + // ends the script) must fall back to the end, not slice backwards. + const end = script.indexOf('\n\n', start); + const helper = script.slice(start, end === -1 ? undefined : end); + assert.ok( + helper.endsWith(';'), + `${name} helper looks truncated — the comparison would be one-sided`, + ); + return helper; + }; + assert.equal( + helperOf(doc.jobs.enforce, 'parseBlocklist'), + helperOf(doc.jobs.sweep, 'parseBlocklist'), + ); + assert.equal( + helperOf(doc.jobs.enforce, 'isBlocked'), + helperOf(doc.jobs.sweep, 'isBlocked'), + ); + assert.equal( + helperOf(doc.jobs.enforce, 'run'), + helperOf(doc.jobs.sweep, 'run'), + ); + assert.equal( + helperOf(doc.jobs.enforce, 'isLocked'), + helperOf(doc.jobs.sweep, 'isLocked'), + ); + }); + + it('checks in a well-formed blocklist', () => { + const entries = readFileSync(join(here, '..', 'spam-blocklist.txt'), 'utf8') + .split('\n') + .map((line) => line.trim()) + .filter((line) => line !== '' && !line.startsWith('#')); + for (const entry of entries) { + assert.equal( + entry, + entry.toLowerCase(), + 'entries are matched lowercased', + ); + // Underscores belong in the charset: legacy GitHub usernames keep + // them, and the workflow parser matches any name it can read — the + // validator must not reject entries the lanes would honour. + assert.match(entry, /^[a-z\d_](?:[a-z\d_]|-(?=[a-z\d])){0,38}$/); + } + }); +}); diff --git a/.github/spam-blocklist.txt b/.github/spam-blocklist.txt index 406bc985432..f835391edf5 100644 --- a/.github/spam-blocklist.txt +++ b/.github/spam-blocklist.txt @@ -1,4 +1,10 @@ -# Users whose comments should be auto-minimized as off-topic. +# Users treated as spam by .github/workflows/spam-blocklist-enforce.yml. # One username per line, case-insensitive. Lines starting with # are comments. -# Add entries here when a user is blocked at the org level or identified as spam. +# +# For every user listed here the workflow deletes their comments, and closes +# and locks the issues and pull requests they open. Only the thread AUTHOR +# matters for closing — a spam comment on someone else's PR is deleted and the +# PR is left open. +# +# Add entries when a user is blocked at the org level or identified as spam. danialzivehdadr diff --git a/.github/workflows/auto-minimize-spam.yml b/.github/workflows/auto-minimize-spam.yml deleted file mode 100644 index 10b05f84186..00000000000 --- a/.github/workflows/auto-minimize-spam.yml +++ /dev/null @@ -1,178 +0,0 @@ -name: 'Auto-minimize spam comments' - -# Periodically scan recent issue/PR comments and minimize any from -# users listed in .github/spam-blocklist.txt. This cleans up spam -# comments that were posted before a block was applied, and catches -# any that slip through during the window between a spam comment -# and the manual block action. -# -# The blocklist is a plain-text file in the repo — one username per -# line, case-insensitive, # for comments. No special API scopes -# needed beyond issues:write + pull-requests:write. -# -# Note: the lookback window (LOOKBACK_HOURS) only filters issues — -# the GraphQL pullRequests connection has no `since` filter, so PRs -# are always scoped to the 100 most recently updated. -# -# Coverage limits: only issue-style comments are scanned — PR review -# (inline) comments, review bodies, and Discussions are not. -# comments(last: 30) means a >30-comment flood on a single thread is -# only partially cleaned per run. - -on: - schedule: - - cron: '30 * * * *' # Every hour at :30 - workflow_dispatch: - inputs: - hours: - description: 'Look back this many hours (default 2)' - required: false - default: 2 - type: 'number' - -permissions: - contents: 'read' - issues: 'write' - pull-requests: 'write' - -concurrency: - group: 'auto-minimize-spam' - cancel-in-progress: false - -jobs: - minimize: - if: "${{ github.repository == 'QwenLM/qwen-code' }}" - runs-on: 'ubuntu-latest' - timeout-minutes: 10 - steps: - - name: 'Checkout blocklist' - uses: 'actions/checkout@df4cb1c069e1874edd31b4311f1884172cec0e10' # v6.0.3 - with: - sparse-checkout: '.github/spam-blocklist.txt' - persist-credentials: false - - - name: 'Minimize comments from blocklisted users' - env: - GH_TOKEN: '${{ secrets.CI_BOT_PAT }}' - LOOKBACK_HOURS: "${{ inputs.hours || '2' }}" - run: |- - set -euo pipefail - - REPO="$GITHUB_REPOSITORY" - SINCE="$(date -u -d "${LOOKBACK_HOURS} hours ago" +%Y-%m-%dT%H:%M:%SZ)" - BLOCKLIST=".github/spam-blocklist.txt" - - write_summary() { - { - echo "## Summary" - echo "- Scanned comments since: ${SINCE}" - echo "- Blocklisted users: ${BLOCKED_COUNT}" - echo "- Comments minimized: $1" - } >> "$GITHUB_STEP_SUMMARY" - } - - echo "Scanning comments since $SINCE in $REPO" - - # ── 1. Parse blocklist ──────────────────────────────────────── - if [ ! -f "$BLOCKLIST" ]; then - echo "::notice::No blocklist file found at $BLOCKLIST; nothing to do." - exit 0 - fi - # Strip comments, blank lines, whitespace; lowercase for matching. - # `|| true` keeps an all-comment/blank blocklist from tripping - # `set -e` (grep exits 1 when nothing passes the filter). - BLOCKED_USERS="$( - grep -v '^\s*#' "$BLOCKLIST" \ - | grep -v '^\s*$' \ - | sed 's/^[[:space:]]*//;s/[[:space:]]*$//' \ - | tr '[:upper:]' '[:lower:]' \ - | sort -u \ - || true - )" - BLOCKED_COUNT="$(printf '%s\n' "$BLOCKED_USERS" | grep -c . || true)" - echo "Blocklisted users: ${BLOCKED_COUNT}" - if [ "$BLOCKED_COUNT" -eq 0 ]; then - echo "Blocklist is empty; nothing to do." - exit 0 - fi - - # ── 2. Fetch recent unminimized comments via GraphQL ────────── - ALL_UNMINIMIZED="$( - gh api graphql -f query=" - query { - repository(owner: \"${REPO%%/*}\", name: \"${REPO##*/}\") { - issues(first: 100, orderBy: {field: UPDATED_AT, direction: DESC}, filterBy: {since: \"${SINCE}\"}) { - nodes { - number - comments(last: 30) { - nodes { id author { login } isMinimized } - } - } - } - pullRequests(first: 100, orderBy: {field: UPDATED_AT, direction: DESC}) { - nodes { - number - comments(last: 30) { - nodes { id author { login } isMinimized } - } - } - } - } - } - " --jq ' - [ - .data.repository.issues.nodes[].comments.nodes[], - .data.repository.pullRequests.nodes[].comments.nodes[] - ] - | map(select(.isMinimized == false and .author != null)) - | .[] | "\(.author.login)\t\(.id)" - ' - )" - - MATCHED_IDS="" - MATCHED_COUNT=0 - while IFS=$'\t' read -r login node_id; do - [ -z "$login" ] && continue - login_lc="$(printf '%s' "$login" | tr '[:upper:]' '[:lower:]')" - if printf '%s\n' "$BLOCKED_USERS" | grep -qxF "$login_lc"; then - MATCHED_IDS="${MATCHED_IDS}${node_id}"$'\n' - MATCHED_COUNT=$((MATCHED_COUNT + 1)) - echo " matched: @${login} → ${node_id}" - fi - done <<< "$ALL_UNMINIMIZED" - - echo "Unminimized comments from blocklisted users: ${MATCHED_COUNT}" - if [ "$MATCHED_COUNT" -eq 0 ]; then - echo "Nothing to minimize." - write_summary 0 - exit 0 - fi - - # ── 3. Minimize each matched comment ────────────────────────── - SUCCESS=0 - FAIL=0 - while IFS= read -r node_id; do - [ -z "$node_id" ] && continue - result="$( - gh api graphql -f query=" - mutation { - minimizeComment(input: {subjectId: \"${node_id}\", classifier: OFF_TOPIC}) { - minimizedComment { isMinimized } - } - } - " --jq '.data.minimizeComment.minimizedComment.isMinimized' 2>&1 - )" || true - if [ "$result" = "true" ]; then - SUCCESS=$((SUCCESS + 1)) - else - FAIL=$((FAIL + 1)) - echo "::warning::Failed to minimize ${node_id}: ${result}" - fi - done <<< "$MATCHED_IDS" - - echo "Minimized ${SUCCESS} comments, ${FAIL} failed." - write_summary "$SUCCESS" - if [ "$FAIL" -gt 0 ]; then - echo "- Failures: ${FAIL}" >> "$GITHUB_STEP_SUMMARY" - exit 1 - fi diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index 1af3e130c07..de7cce225be 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -50,7 +50,7 @@ env: # BOTH the github_ci_only helper step and the full-profile Test step, so a # new helper test can't be added to one path and silently dropped from the # other. - HELPER_TESTS: '.github/scripts/pr-safety-precheck.test.mjs .github/scripts/cap-release-notes.test.mjs .github/scripts/ci/classify-profile.test.mjs .github/scripts/ci/classify-pr-profile.test.mjs .github/scripts/upsert-bot-comment.test.mjs .github/scripts/ci/main-failure-signature.test.mjs .github/scripts/classify-release-notes.test.mjs .github/scripts/dsw-swe-verified/make-manifest.test.mjs .github/scripts/resolve-sandbox-image.test.mjs .github/scripts/web-shell-visuals-publish.test.mjs .github/scripts/web-shell-visuals-compose.test.mjs .github/scripts/serve-ab-diff.test.mjs .github/scripts/qwen-triage-workflow.test.mjs .github/scripts/assign-issue-owner.test.mjs .github/scripts/auto-minimize-spam.test.mjs .github/scripts/ci-runner-routing.test.mjs' + HELPER_TESTS: '.github/scripts/pr-safety-precheck.test.mjs .github/scripts/cap-release-notes.test.mjs .github/scripts/ci/classify-profile.test.mjs .github/scripts/ci/classify-pr-profile.test.mjs .github/scripts/upsert-bot-comment.test.mjs .github/scripts/ci/main-failure-signature.test.mjs .github/scripts/classify-release-notes.test.mjs .github/scripts/dsw-swe-verified/make-manifest.test.mjs .github/scripts/resolve-sandbox-image.test.mjs .github/scripts/web-shell-visuals-publish.test.mjs .github/scripts/web-shell-visuals-compose.test.mjs .github/scripts/serve-ab-diff.test.mjs .github/scripts/qwen-triage-workflow.test.mjs .github/scripts/assign-issue-owner.test.mjs .github/scripts/spam-blocklist-enforce.test.mjs .github/scripts/ci-runner-routing.test.mjs' jobs: classify_pr: diff --git a/.github/workflows/spam-blocklist-enforce.yml b/.github/workflows/spam-blocklist-enforce.yml new file mode 100644 index 00000000000..bfddcd380db --- /dev/null +++ b/.github/workflows/spam-blocklist-enforce.yml @@ -0,0 +1,648 @@ +name: 'Spam blocklist enforcement' + +# Enforce .github/spam-blocklist.txt: delete comments authored by blocklisted +# users, and close + lock the issues/PRs they open. +# +# The blocklist is a plain-text file in the repo — one username per line, +# case-insensitive, # for comments. +# +# Two lanes: +# enforce — event-driven, fires on the comment/thread that just landed so +# spam disappears in seconds rather than at the next sweep. +# sweep — hourly backstop for anything the event lane missed (a comment +# posted before the username was added to the blocklist, or an +# enforce run that failed). It scans the last two hours by +# default; spam older than the window needs a one-off +# workflow_dispatch with a larger `hours` input, because the +# listings filter on last-updated time and never re-see it. +# +# Only the thread AUTHOR being blocklisted closes a thread. A spam comment on +# someone else's PR deletes the comment and leaves the PR open — closing it +# would punish the legitimate author. +# +# Deletion runs on the default GITHUB_TOKEN. The predecessor of this workflow +# minimized comments instead, which needs the GraphQL `minimizeComment` +# mutation and therefore a PAT with the full `repo` scope; the PAT it used had +# only `public_repo`, so every run failed with INSUFFICIENT_SCOPES and no spam +# was ever hidden. REST delete needs nothing beyond issues:write + +# pull-requests:write, which GITHUB_TOKEN grants. +# +# Coverage limit: a review BODY (as opposed to an inline review comment) has no +# REST delete endpoint, so the event lane minimizes it as SPAM instead and the +# sweep skips it — there is no repo-wide "list reviews" endpoint to sweep with. +# A review BODY on a fork PR has no lane at all: the review events run on a +# read-only token there and the sweep cannot list reviews, so it needs manual +# minimization. A closed+locked PR's body also stays editable by its author — +# locking restricts comments, not body edits — so spam edited into a finished +# PR's body is manual cleanup too. + +on: + issue_comment: + types: + - 'created' + - 'edited' + pull_request_review_comment: + types: + - 'created' + - 'edited' + pull_request_review: + types: + - 'submitted' + - 'edited' + # `dismissed` is the one type that reaches a review posted before its + # author was blocklisted: a maintainer dismissing it is the signal to + # minimize it. + - 'dismissed' + # Deliberately NOT `issues:`. The repository holds qwen-triage to being the + # single immediate owner of issue opened/reopened/edited, enforced by + # scripts/tests/issue-triage-ownership-workflow.test.js — a second workflow + # racing it on issue open is exactly what that invariant exists to prevent. + # A spam issue is therefore closed by the sweep lane within the hour rather + # than instantly. Spam pull requests keep the instant lane below, and a spam + # issue that its author then comments on is closed by the issue_comment lane. + # + # pull_request_target, not pull_request: a fork PR from a blocklisted user + # must be closable, and `pull_request` grants a read-only token on forks. + # Nothing here checks out or executes PR code — see the pinned checkout ref + # below — so the elevated token is not exposed to the PR branch. + pull_request_target: + types: + - 'opened' + - 'reopened' + schedule: + - cron: '30 * * * *' # Every hour at :30 + workflow_dispatch: + inputs: + hours: + description: 'Sweep lane: hours to look back (default 2); raise it to reach spam older than the normal window' + required: false + default: 2 + type: 'number' + +permissions: + contents: 'read' + issues: 'write' + pull-requests: 'write' + +jobs: + enforce: + if: "${{ github.repository == 'QwenLM/qwen-code' && github.event_name != 'schedule' && github.event_name != 'workflow_dispatch' }}" + runs-on: 'ubuntu-latest' + timeout-minutes: 5 + concurrency: + # Keyed on the subject this event is about so unrelated spam is handled + # in parallel, and a comment plus its own edit are serialised. Not + # cancel-in-progress: a cancelled run leaves the spam standing. + group: >- + spam-blocklist-enforce-${{ + github.event.comment.id || github.event.review.id || + github.event.issue.number || github.event.pull_request.number || + github.run_id + }} + cancel-in-progress: false + steps: + - name: 'Checkout blocklist' + uses: 'actions/checkout@df4cb1c069e1874edd31b4311f1884172cec0e10' # v6.0.3 + with: + # Pinned to the default branch, never the event's ref. On + # pull_request_target the default would be the base branch anyway, + # but stating it makes it impossible for a future edit to start + # reading the blocklist out of a PR head. + ref: '${{ github.event.repository.default_branch }}' + sparse-checkout: '.github/spam-blocklist.txt' + persist-credentials: false + + - name: 'Enforce blocklist on this event' + uses: 'actions/github-script@3a2844b7e9c422d3c10d287c895573f7108da1b3' # v9.0.0 + env: + BLOCKLIST_PATH: '.github/spam-blocklist.txt' + with: + github-token: '${{ secrets.GITHUB_TOKEN }}' + script: | + const { readFileSync } = require('node:fs'); + + const parseBlocklist = (text) => + new Set( + text + .split('\n') + .map((line) => line.trim()) + .filter((line) => line !== '' && !line.startsWith('#')) + .map((line) => line.toLowerCase()), + ); + + let blocked; + try { + blocked = parseBlocklist( + readFileSync(process.env.BLOCKLIST_PATH, 'utf8'), + ); + } catch (error) { + core.info(`No blocklist at ${process.env.BLOCKLIST_PATH}; nothing to do.`); + return; + } + if (blocked.size === 0) { + core.info('Blocklist is empty; nothing to do.'); + return; + } + const isBlocked = (login) => + typeof login === 'string' && blocked.has(login.toLowerCase()); + + const { owner, repo } = context.repo; + const eventName = context.eventName; + const payload = context.payload; + const actions = []; + const failures = []; + + // Tolerated statuses are per-operation. A 404 proves the target + // is already gone — the desired end state for the delete and + // close calls below. A 422 proves nothing — GitHub also uses it + // for validation and abuse-protection failures — so only the + // lock call tolerates one, and only after the lock state reads + // back locked. + const run = async (label, fn, tolerated = [404]) => { + try { + const result = await fn(); + actions.push(label); + core.info(`ok: ${label}`); + return result; + } catch (error) { + if (tolerated.includes(error?.status)) { + core.info(`already gone: ${label}`); + return undefined; + } + failures.push(`${label}: ${error?.status ?? ''} ${error?.message ?? error}`); + core.warning(`failed: ${label} — ${error?.message ?? error}`); + return undefined; + } + }; + + // issues.lock answers 422 both for a thread a concurrent run + // already locked and for validation/abuse failures. Reading the + // lock state back tells the two apart; an unreadable thread + // fails closed. + const isLocked = async (number) => { + try { + const { data } = await github.rest.issues.get({ + owner, + repo, + issue_number: number, + }); + return data?.locked === true; + } catch { + return false; + } + }; + + // Close + lock a thread opened by a blocklisted user. Issues close + // through issues.update so they can carry state_reason; PRs close + // through pulls.update, which is the endpoint that owns PR state. + // Locking is issues.lock for both — a PR is an issue as far as + // conversation locking is concerned. A thread a previous run + // closed but failed to lock still needs its lock, so skip only + // the close half when it is already closed. + const closeThread = async (number, isPullRequest, state) => { + const kind = isPullRequest ? 'pull request' : 'issue'; + if (state !== 'closed') { + await run(`close ${kind} #${number}`, () => + isPullRequest + ? github.rest.pulls.update({ + owner, + repo, + pull_number: number, + state: 'closed', + }) + : github.rest.issues.update({ + owner, + repo, + issue_number: number, + state: 'closed', + state_reason: 'not_planned', + }), + ); + } + await run(`lock ${kind} #${number}`, async () => { + try { + await github.rest.issues.lock({ + owner, + repo, + issue_number: number, + lock_reason: 'spam', + }); + } catch (error) { + // A 422 lock race counts only when the thread reads back + // locked; any other 422 rethrows and fails the run. + const lockRace = + error?.status === 422 && (await isLocked(number)); + if (!lockRace) throw error; + } + }); + }; + + if (eventName === 'issue_comment') { + const comment = payload.comment; + const issue = payload.issue; + // Issue-comment events on fork PRs run on a read-only + // GITHUB_TOKEN just like the review lanes; every write below + // would 403 and red-run this one. Defer to the lanes holding + // a write token: pull_request_target closes fork PRs at open + // time, the hourly sweep deletes comments and repairs thread + // state. The payload carries no top-level pull_request — + // PR-ness sits on issue.pull_request and the head repo must + // be resolved with pulls.get. A deleted fork reads back as + // head.repo: null with the same read-only downgrade, so a + // missing head repo counts as a fork. + if ( + issue?.pull_request && + (isBlocked(comment?.user?.login) || + isBlocked(issue?.user?.login)) + ) { + const { data: pr } = await github.rest.pulls.get({ + owner, + repo, + pull_number: issue.number, + }); + if ( + !pr.head?.repo || + pr.head.repo.full_name !== `${owner}/${repo}` + ) { + core.notice( + 'fork PR: GITHUB_TOKEN is read-only here; deferring to the sweep lane', + ); + return; + } + } + if (isBlocked(comment?.user?.login)) { + await run(`delete issue comment ${comment.id}`, () => + github.rest.issues.deleteComment({ + owner, + repo, + comment_id: comment.id, + }), + ); + } + // The comment may have arrived on a thread the spammer also + // opened — e.g. they bump their own spam issue. Handle the + // thread here so the sweep is not the only path that closes it. + // Skip only the finished state: a closed thread whose lock + // failed still needs the lock, and a locked thread whose close + // failed still needs the close. + if ( + issue && + !(issue.state === 'closed' && issue.locked) && + isBlocked(issue.user?.login) + ) { + await closeThread(issue.number, Boolean(issue.pull_request), issue.state); + } + } else if (eventName === 'pull_request_review_comment') { + const comment = payload.comment; + const pull = payload.pull_request; + // Review events on a fork PR run on a read-only GITHUB_TOKEN; + // every write below would 403 and red-run the lane. Defer to + // the lanes holding a write token: pull_request_target closes + // fork PRs at open time, the hourly sweep deletes comments and + // repairs thread state. A deleted fork arrives as + // head.repo: null with the same read-only downgrade, so a + // missing head repo counts as a fork. + if ( + pull && + (!pull.head?.repo || + pull.head.repo.full_name !== `${owner}/${repo}`) && + (isBlocked(comment?.user?.login) || + isBlocked(pull.user?.login)) + ) { + core.notice( + 'fork PR: GITHUB_TOKEN is read-only here; deferring to the sweep lane', + ); + return; + } + if (isBlocked(comment?.user?.login)) { + await run(`delete review comment ${comment.id}`, () => + github.rest.pulls.deleteReviewComment({ + owner, + repo, + comment_id: comment.id, + }), + ); + } + // Same thread-author close as issue_comment: a legitimate + // review on a blocklisted author's PR still closes the PR, + // instead of leaving it to the sweep. + if ( + pull && + !(pull.state === 'closed' && pull.locked) && + isBlocked(pull.user?.login) + ) { + await closeThread(pull.number, true, pull.state); + } + } else if (eventName === 'pull_request_review') { + const review = payload.review; + const pull = payload.pull_request; + // Same read-only downgrade as the review-comment lane — except + // a review BODY has no sweep backstop, so the notice is all the + // automation leaves behind: the body needs manual minimization. + // A deleted fork arrives as head.repo: null and guards the + // same way as a live one. + if ( + pull && + (!pull.head?.repo || + pull.head.repo.full_name !== `${owner}/${repo}`) && + (isBlocked(review?.user?.login) || + isBlocked(pull.user?.login)) + ) { + core.notice( + 'fork PR: GITHUB_TOKEN is read-only here; the review body needs manual minimization', + ); + return; + } + if (isBlocked(review?.user?.login)) { + // No REST delete for a review body; SPAM-classify it instead. + // The minimize tolerates no status: a failed minimize leaves + // the body visible with no sweep backstop, so it must + // red-run. + const minimize = () => + github.graphql( + `mutation MinimizeComment($id: ID!) { + minimizeComment(input: {subjectId: $id, classifier: SPAM}) { + minimizedComment { isMinimized } + } + }`, + { id: review.node_id }, + ); + await run(`minimize review ${review.id}`, minimize, []); + } + if ( + pull && + !(pull.state === 'closed' && pull.locked) && + isBlocked(pull.user?.login) + ) { + await closeThread(pull.number, true, pull.state); + } + } else if (eventName === 'pull_request_target') { + const pull = payload.pull_request; + if (isBlocked(pull?.user?.login)) { + await closeThread(pull.number, true, pull.state); + } + } + + // Both empty means either no blocklisted author was involved or + // every needed action was already done (a tolerated status + // above). Checking `failures` too matters: when the single + // action attempted is the one that failed, `actions` is still + // empty, and returning here would skip the setFailed below and + // report the run green. + if (actions.length === 0 && failures.length === 0) { + core.info('No actions taken; any needed work was already done.'); + return; + } + + await core.summary + .addHeading('Spam blocklist enforcement') + .addList([...actions, ...failures.map((f) => `FAILED — ${f}`)]) + .write(); + + if (failures.length > 0) { + core.setFailed(failures.join('\n')); + } + + sweep: + if: "${{ github.repository == 'QwenLM/qwen-code' && (github.event_name == 'schedule' || github.event_name == 'workflow_dispatch') }}" + runs-on: 'ubuntu-latest' + timeout-minutes: 10 + concurrency: + group: 'spam-blocklist-sweep' + cancel-in-progress: false + steps: + - name: 'Checkout blocklist' + uses: 'actions/checkout@df4cb1c069e1874edd31b4311f1884172cec0e10' # v6.0.3 + with: + ref: '${{ github.event.repository.default_branch }}' + sparse-checkout: '.github/spam-blocklist.txt' + persist-credentials: false + + - name: 'Sweep recent activity for blocklisted authors' + uses: 'actions/github-script@3a2844b7e9c422d3c10d287c895573f7108da1b3' # v9.0.0 + env: + BLOCKLIST_PATH: '.github/spam-blocklist.txt' + LOOKBACK_HOURS: "${{ inputs.hours || '2' }}" + with: + github-token: '${{ secrets.GITHUB_TOKEN }}' + script: | + const { readFileSync } = require('node:fs'); + + const parseBlocklist = (text) => + new Set( + text + .split('\n') + .map((line) => line.trim()) + .filter((line) => line !== '' && !line.startsWith('#')) + .map((line) => line.toLowerCase()), + ); + + let blocked; + try { + blocked = parseBlocklist( + readFileSync(process.env.BLOCKLIST_PATH, 'utf8'), + ); + } catch { + core.info(`No blocklist at ${process.env.BLOCKLIST_PATH}; nothing to do.`); + return; + } + if (blocked.size === 0) { + core.info('Blocklist is empty; nothing to do.'); + return; + } + const isBlocked = (login) => + typeof login === 'string' && blocked.has(login.toLowerCase()); + + const { owner, repo } = context.repo; + const hours = Number(process.env.LOOKBACK_HOURS) || 2; + const since = new Date(Date.now() - hours * 3600 * 1000).toISOString(); + core.info(`Sweeping ${owner}/${repo} since ${since} for ${blocked.size} blocklisted user(s).`); + + const actions = []; + const failures = []; + // Tolerated statuses are per-operation. A 404 proves the target + // is already gone — the desired end state for the delete, close, + // and lock calls below. A 422 proves nothing — GitHub also uses + // it for validation and abuse-protection failures — so only the + // lock call tolerates one, and only after the lock state reads + // back locked. The listings pass [] and fail on every status. + const run = async (label, fn, tolerated = [404]) => { + try { + const result = await fn(); + actions.push(label); + core.info(`ok: ${label}`); + return result; + } catch (error) { + if (tolerated.includes(error?.status)) { + core.info(`already gone: ${label}`); + return undefined; + } + failures.push(`${label}: ${error?.status ?? ''} ${error?.message ?? error}`); + core.warning(`failed: ${label} — ${error?.message ?? error}`); + return undefined; + } + }; + + // issues.lock answers 422 both for a thread a concurrent run + // already locked and for validation/abuse failures. Reading the + // lock state back tells the two apart; an unreadable thread + // fails closed. + const isLocked = async (number) => { + try { + const { data } = await github.rest.issues.get({ + owner, + repo, + issue_number: number, + }); + return data?.locked === true; + } catch { + return false; + } + }; + + // Repo-wide comment listings, not a walk over recently-updated + // threads: they honour `since` directly, so a spam comment on a + // year-old thread is still in scope, and they are the only listing + // that surfaces inline review comments at all. The predecessor + // workflow walked threads and consequently never saw a single + // inline review comment. Routed through run() so a failed listing + // (a rate-limit 403 is realistic — the token's limit is shared + // with every enforce run of the same hour) collects as a failure + // and the rest of the sweep still executes. A listing tolerates + // no status: neither a 404 nor a 422 can prove there was nothing + // to scan, so any non-success response fails the run instead of + // silently scanning zero items. + const issueComments = + (await run( + 'list issue comments', + () => + github.paginate(github.rest.issues.listCommentsForRepo, { + owner, + repo, + since, + per_page: 100, + }), + [], + )) ?? []; + for (const comment of issueComments) { + if (!isBlocked(comment.user?.login)) continue; + await run(`delete issue comment ${comment.id}`, () => + github.rest.issues.deleteComment({ + owner, + repo, + comment_id: comment.id, + }), + ); + } + + const reviewComments = + (await run( + 'list review comments', + () => + github.paginate(github.rest.pulls.listReviewCommentsForRepo, { + owner, + repo, + since, + per_page: 100, + }), + [], + )) ?? []; + for (const comment of reviewComments) { + if (!isBlocked(comment.user?.login)) continue; + await run(`delete review comment ${comment.id}`, () => + github.rest.pulls.deleteReviewComment({ + owner, + repo, + comment_id: comment.id, + }), + ); + } + + // listForRepo returns issues AND pull requests. `state: 'all'` + // matters: a thread a previous run closed but failed to lock is + // exactly what this backstop must repair, and `state: 'open'` + // would filter it out. `locked` is not a skip condition either: + // the mirror leftover — a close that failed before the lock + // landed — is open AND locked, and only the close half repairs + // it (a blocklisted author can also reach that state alone by + // reopening their own locked thread). `locked` therefore guards + // only the lock call; everything still open gets closed. + const threads = + (await run( + 'list threads', + () => + github.paginate(github.rest.issues.listForRepo, { + owner, + repo, + state: 'all', + since, + sort: 'updated', + direction: 'desc', + per_page: 100, + }), + [], + )) ?? []; + for (const thread of threads) { + if (!isBlocked(thread.user?.login)) continue; + const isPullRequest = Boolean(thread.pull_request); + const kind = isPullRequest ? 'pull request' : 'issue'; + if (thread.state === 'open') { + await run(`close ${kind} #${thread.number}`, () => + isPullRequest + ? github.rest.pulls.update({ + owner, + repo, + pull_number: thread.number, + state: 'closed', + }) + : github.rest.issues.update({ + owner, + repo, + issue_number: thread.number, + state: 'closed', + state_reason: 'not_planned', + }), + ); + } + if (!thread.locked) { + await run(`lock ${kind} #${thread.number}`, async () => { + try { + await github.rest.issues.lock({ + owner, + repo, + issue_number: thread.number, + lock_reason: 'spam', + }); + } catch (error) { + // A 422 lock race counts only when the thread reads + // back locked; any other 422 rethrows and fails the + // run. + const lockRace = + error?.status === 422 && + (await isLocked(thread.number)); + if (!lockRace) throw error; + } + }); + } + } + + await core.summary + .addHeading('Spam blocklist sweep') + .addTable([ + [ + { data: 'Field', header: true }, + { data: 'Value', header: true }, + ], + ['Since', since], + ['Blocklisted users', String(blocked.size)], + ['Issue comments scanned', String(issueComments.length)], + ['Review comments scanned', String(reviewComments.length)], + ['Threads scanned', String(threads.length)], + ['Actions taken', String(actions.length)], + ]) + .addList([...actions, ...failures.map((f) => `FAILED — ${f}`)]) + .write(); + + if (failures.length > 0) { + core.setFailed(failures.join('\n')); + } diff --git a/packages/cli/src/commands/review/compose-review.test.ts b/packages/cli/src/commands/review/compose-review.test.ts index d89d932cac9..925747c3bcc 100644 --- a/packages/cli/src/commands/review/compose-review.test.ts +++ b/packages/cli/src/commands/review/compose-review.test.ts @@ -721,7 +721,7 @@ describe('repository context proof boundary', () => { }); it('caps the unverified-dimension disclosure at five entries', () => { - // The schema admits 128 dimensions x 512 chars; joined into one + // The schema admits 256 dimensions x 512 chars; joined into one // disclosure that outruns the review body's own size budget — the same // cap discipline testPlanGate applies to its notes. const planPath = join(dir, 'capped-plan.json'); diff --git a/packages/cli/src/commands/review/compose-review.ts b/packages/cli/src/commands/review/compose-review.ts index da1a62c3868..10295291ecd 100644 --- a/packages/cli/src/commands/review/compose-review.ts +++ b/packages/cli/src/commands/review/compose-review.ts @@ -1892,7 +1892,7 @@ export function repositoryContextGate(planPath: string): string[] { const dimensions = context?.unverifiedDimensions ?? []; // The same cap discipline testPlanGate applies: unbounded entries joined // into one disclosure drown the verdict they ride on — and at the schema - // bounds (128 x 512 chars) the paragraph outruns the review body's own + // bounds (256 x 512 chars) the paragraph outruns the review body's own // budget before any other content gets a word in. const MAX_DIMENSIONS = 5; const disclosed = dimensions diff --git a/packages/cli/src/commands/review/lib/manifest-repository-context.test.ts b/packages/cli/src/commands/review/lib/manifest-repository-context.test.ts index 875e97b62d9..4d6a432a368 100644 --- a/packages/cli/src/commands/review/lib/manifest-repository-context.test.ts +++ b/packages/cli/src/commands/review/lib/manifest-repository-context.test.ts @@ -225,14 +225,12 @@ describe('manifest repository context provider', () => { worktree, ['src/change.ts'], manifest({ - rules: Array.from({ length: 65 }, (_, index) => ({ + rules: Array.from({ length: 2 }, (_, rule) => ({ paths: ['src/**'], - verificationNotes: [ - `note-a-${String(index).padStart(3, '0')}`, - `note-b-${String(index).padStart(3, '0')}`, - `note-c-${String(index).padStart(3, '0')}`, - `note-d-${String(index).padStart(3, '0')}`, - ], + verificationNotes: Array.from( + { length: 129 }, + (_, index) => `note-${rule}-${String(index).padStart(3, '0')}`, + ), })), }), ), @@ -579,13 +577,19 @@ describe('manifest repository context provider', () => { for (let index = 0; index < 4; index++) { write(join(worktree, 'docs', `${index}.ts`)); } + for (let index = 0; index < 2; index++) { + write(join(worktree, 'misc', `${index}.ts`)); + } + // 128 === MAX_RULES: parsing at exactly the bound doubles as the + // accept-side pin for the provider's `>` check. `extra/**` does not + // exist: it keeps the scan-root ENOENT skip branch exercised. const rules = Array.from({ length: 128 }, () => ({ paths: ['src/**'], - relatedPaths: ['src/**', 'docs/**', 'extra/**'], + relatedPaths: ['src/**', 'docs/**', 'misc/**', 'extra/**'], })); expect( provide(worktree, ['src/change.ts'], manifest({ rules }))?.relatedPaths, - ).toHaveLength(9); + ).toHaveLength(11); }); it('fails closed when cumulative matching work exceeds the budget', () => {