From bd6f0f22de4aedfaf8146d81e6f486c28200c0fd Mon Sep 17 00:00:00 2001 From: wenshao Date: Sun, 9 Aug 2026 07:24:06 +0800 Subject: [PATCH 01/10] fix(ci): make spam blocklist enforcement actually work MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Replace the auto-minimize-spam workflow with one that deletes blocklisted users' comments and closes the pull requests they open. The old workflow minimized comments through the GraphQL minimizeComment mutation, which requires a token with the full repo scope. The PAT it used carried only public_repo, so every run since the blocklist became non-empty failed with INSUFFICIENT_SCOPES and not one spam comment was ever hidden. REST deletion needs nothing beyond issues:write and pull-requests:write, so enforcement now runs on the default GITHUB_TOKEN and the PAT is gone. Enforcement also gained an event-driven lane, so spam disappears within seconds of being posted rather than at the next hourly sweep, and it now covers inline review comments and review bodies, which the thread-walking scan could never see. Only the thread author being blocklisted closes a thread — a spam comment on someone else's pull request is deleted and the pull request left open. The event lane deliberately does not subscribe to the issues event, since qwen-triage is held to being the single immediate owner of issue opened/reopened/edited. Spam issues are closed by the sweep within the hour instead, and a guard here fails loudly if that trigger is ever restored. --- .github/scripts/auto-minimize-spam.test.mjs | 73 --- .../scripts/spam-blocklist-enforce.test.mjs | 538 ++++++++++++++++++ .github/spam-blocklist.txt | 10 +- .github/workflows/auto-minimize-spam.yml | 178 ------ .github/workflows/ci.yml | 2 +- .github/workflows/spam-blocklist-enforce.yml | 432 ++++++++++++++ 6 files changed, 979 insertions(+), 254 deletions(-) delete mode 100644 .github/scripts/auto-minimize-spam.test.mjs create mode 100644 .github/scripts/spam-blocklist-enforce.test.mjs delete mode 100644 .github/workflows/auto-minimize-spam.yml create mode 100644 .github/workflows/spam-blocklist-enforce.yml 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..245612979b4 --- /dev/null +++ b/.github/scripts/spam-blocklist-enforce.test.mjs @@ -0,0 +1,538 @@ +// 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); + +const jobs = ['enforce', 'sweep'].map((name) => [name, doc.jobs[name]]); +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: 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. + assert.match( + String(doc.jobs.enforce.if), + /github\.event_name != 'schedule' && github\.event_name != 'workflow_dispatch'/, + ); + assert.match( + String(doc.jobs.sweep.if), + /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 }}', + ); + assert.equal( + checkout.with['sparse-checkout'], + '.github/spam-blocklist.txt', + ); + }); + + 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', + ); + 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', () => { + // minimizeComment needs a PAT with the full `repo` scope; REST delete + // needs nothing beyond the permissions block above. A PAT reappearing + // here almost certainly means the workflow has gone back to minimizing, + // and back to failing on an under-scoped token. + assert.doesNotMatch(source, /secrets\.CI_BOT_PAT/); + }); +}); + +describe('spam-blocklist-enforce: event coverage', () => { + it('listens on every surface a blocklisted user can post from', () => { + 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('keeps a scheduled backstop', () => { + assert.ok(Array.isArray(doc.on.schedule) && doc.on.schedule.length > 0); + }); +}); + +// ── 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: [], failed: [] }; + const summary = { + addHeading: () => summary, + addList: () => summary, + addTable: () => summary, + write: async () => summary, + }; + return { + logs, + info: (m) => logs.info.push(m), + warning: (m) => logs.warning.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. +const makeGithub = ({ calls, fail = () => null, pages = {} }) => { + const record = (name) => async (params) => { + calls.push({ name, params }); + const error = fail(name, params); + if (error) throw error; + return { data: {} }; + }; + return { + rest: { + issues: { + deleteComment: record('issues.deleteComment'), + update: record('issues.update'), + lock: record('issues.lock'), + // 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'), + listReviewCommentsForRepo: 'pulls.listReviewCommentsForRepo', + }, + }, + graphql: async (_query, variables) => { + calls.push({ name: 'graphql.minimizeComment', params: variables }); + const error = fail('graphql.minimizeComment', variables); + if (error) throw error; + return {}; + }, + paginate: async (endpoint, params) => { + calls.push({ name: `paginate:${endpoint}`, params }); + 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 enforce = async ( + eventName, + payload, + { blocklist = BLOCKLIST, fail } = {}, +) => { + const calls = []; + const core = makeCore(); + await runLane('enforce', { + eventName, + payload, + env: { BLOCKLIST_PATH: blocklist }, + github: makeGithub({ calls, fail }), + 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 blocklisted author/.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: {}, + }, + }); + assert.deepEqual(names(calls), ['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' } }, + issue: { number: 42, user: { login: 'spamuser' }, state: 'open' }, + }); + assert.deepEqual(names(calls), [ + 'issues.deleteComment', + 'issues.update', + 'issues.lock', + ]); + assert.equal(calls[1].params.state, 'closed'); + assert.equal(calls[1].params.state_reason, 'not_planned'); + assert.equal(calls[2].params.lock_reason, 'spam'); + }); + + it('does not re-close an already-closed thread', async () => { + 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']); + }); + + 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('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'); + }); + + it('ignores an issues event, which this workflow no longer subscribes to', async () => { + // Belt and braces alongside the `on:` assertion above: if the trigger is + // ever restored, the script must not silently do nothing. + const { calls } = await enforce('issues', { + issue: { number: 100, user: { login: 'other' } }, + }); + assert.deepEqual(names(calls), []); + }); + + it('closes a fork PR through pulls.update but locks through issues.lock', async () => { + const { calls } = await enforce('pull_request_target', { + pull_request: { number: 101, user: { login: 'spamuser' } }, + }); + 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('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('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/); + }); + + 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.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.ok(core.logs.info.some((m) => /No blocklist/.test(m))); + }); + + it('survives a ghost author on a deleted account', async () => { + const { calls } = await enforce('issue_comment', { + comment: { id: 1, user: null }, + issue: { number: 1, user: null, state: 'open' }, + }); + assert.deepEqual(names(calls), []); + }); +}); + +describe('spam-blocklist-enforce: sweep lane behaviour', () => { + const sweep = async ({ + issueComments = [], + reviewComments = [], + threads = [], + fail, + } = {}) => { + const calls = []; + const core = makeCore(); + await runLane('sweep', { + eventName: 'schedule', + payload: {}, + env: { BLOCKLIST_PATH: BLOCKLIST, LOOKBACK_HOURS: '2' }, + github: makeGithub({ + calls, + fail, + 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('closes blocklisted-authored threads, routing PRs to pulls.update', () => + sweep({ + threads: [ + { number: 10, user: { login: 'legit' } }, + { number: 11, user: { login: 'spamuser' } }, + { number: 12, user: { login: 'other' }, pull_request: { url: 'x' } }, + ], + }).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[2].params.pull_number, 12); + })); + + it('scopes all three listings to the lookback window', async () => { + 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, + 'open', + ); + }); +}); + +describe('spam-blocklist-enforce: blocklist file', () => { + it('embeds the same parser in both lanes', () => { + assert.equal(source.split('const parseBlocklist =').length - 1, 2); + }); + + 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('#')); + assert.ok(entries.length > 0, 'blocklist should not be empty'); + for (const entry of entries) { + assert.equal( + entry, + entry.toLowerCase(), + 'entries are matched lowercased', + ); + 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 ae311d193c4..3bc29e5107f 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..ac75ceeec2c --- /dev/null +++ b/.github/workflows/spam-blocklist-enforce.yml @@ -0,0 +1,432 @@ +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). +# +# 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. + +on: + issue_comment: + types: + - 'created' + - 'edited' + pull_request_review_comment: + types: + - 'created' + - 'edited' + pull_request_review: + types: + - 'submitted' + - 'edited' + # 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: look back this many hours (default 2)' + 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 = []; + + const run = async (label, fn) => { + try { + await fn(); + actions.push(label); + core.info(`ok: ${label}`); + } catch (error) { + // 404 means someone (a maintainer, the spammer, a concurrent + // run of this workflow) already removed it. That is the + // desired end state, so it is not a failure. + if (error?.status === 404) { + core.info(`already gone: ${label}`); + return; + } + failures.push(`${label}: ${error?.status ?? ''} ${error?.message ?? error}`); + core.warning(`failed: ${label} — ${error?.message ?? error}`); + } + }; + + // 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. + const closeThread = async (number, isPullRequest) => { + const kind = isPullRequest ? 'pull request' : 'issue'; + 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}`, () => + github.rest.issues.lock({ + owner, + repo, + issue_number: number, + lock_reason: 'spam', + }), + ); + }; + + if (eventName === 'issue_comment') { + const comment = payload.comment; + 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. + const issue = payload.issue; + if (issue && issue.state !== 'closed' && isBlocked(issue.user?.login)) { + await closeThread(issue.number, Boolean(issue.pull_request)); + } + } else if (eventName === 'pull_request_review_comment') { + const comment = payload.comment; + if (isBlocked(comment?.user?.login)) { + await run(`delete review comment ${comment.id}`, () => + github.rest.pulls.deleteReviewComment({ + owner, + repo, + comment_id: comment.id, + }), + ); + } + } else if (eventName === 'pull_request_review') { + const review = payload.review; + if (isBlocked(review?.user?.login)) { + // No REST delete for a review body; SPAM-classify it instead. + await run(`minimize review ${review.id}`, () => + github.graphql( + `mutation MinimizeComment($id: ID!) { + minimizeComment(input: {subjectId: $id, classifier: SPAM}) { + minimizedComment { isMinimized } + } + }`, + { id: review.node_id }, + ), + ); + } + } else if (eventName === 'pull_request_target') { + const pull = payload.pull_request; + if (isBlocked(pull?.user?.login)) { + await closeThread(pull.number, true); + } + } + + // Both empty means the author was not blocklisted. 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 blocklisted author on this event.'); + 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 = []; + const run = async (label, fn) => { + try { + await fn(); + actions.push(label); + core.info(`ok: ${label}`); + } catch (error) { + if (error?.status === 404) { + core.info(`already gone: ${label}`); + return; + } + failures.push(`${label}: ${error?.status ?? ''} ${error?.message ?? error}`); + core.warning(`failed: ${label} — ${error?.message ?? error}`); + } + }; + + // 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. + const issueComments = await 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 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: 'open'` + // keeps the sweep from re-locking threads that were already dealt + // with on a previous run. + const threads = await github.paginate(github.rest.issues.listForRepo, { + owner, + repo, + state: 'open', + 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'; + 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', + }), + ); + await run(`lock ${kind} #${thread.number}`, () => + github.rest.issues.lock({ + owner, + repo, + issue_number: thread.number, + lock_reason: 'spam', + }), + ); + } + + 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)], + ['Open threads scanned', String(threads.length)], + ['Actions taken', String(actions.length)], + ]) + .addList(actions) + .write(); + + if (failures.length > 0) { + core.setFailed(failures.join('\n')); + } From adc854cb9ed480742b631562d2e294e88033b6d1 Mon Sep 17 00:00:00 2001 From: qwen-code-dev-bot Date: Wed, 12 Aug 2026 15:32:36 +0000 Subject: [PATCH 02/10] fix(ci): harden spam blocklist guard tests and repair sweep gaps (#8767) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Pin every mutation-probed invariant the reviewers flagged: sweep close/lock params, legit-author no-ops for all lanes, full if: routing expressions, per-event types, cron cadence, parser parity (body comparison, not occurrence count), step env wiring, and the checkout repository input. Fix the defects behind the probes: the sweep now retries locks on closed-but-unlocked threads (state 'all' + skip-on-locked), routes its three listings through run() so a failed listing cannot abort the lane, surfaces failures in the step summary, and the event lane listens for review dismissals — the one trigger that reaches pre-blocklist review bodies. The blocklist validator now accepts legacy underscore usernames. --- .../scripts/spam-blocklist-enforce.test.mjs | 387 ++++++++++++++++-- .github/workflows/spam-blocklist-enforce.yml | 164 +++++--- 2 files changed, 457 insertions(+), 94 deletions(-) diff --git a/.github/scripts/spam-blocklist-enforce.test.mjs b/.github/scripts/spam-blocklist-enforce.test.mjs index 245612979b4..19a6e8a009e 100644 --- a/.github/scripts/spam-blocklist-enforce.test.mjs +++ b/.github/scripts/spam-blocklist-enforce.test.mjs @@ -33,7 +33,9 @@ const workflowPath = join( const source = readFileSync(workflowPath, 'utf8'); const doc = parse(source); -const jobs = ['enforce', 'sweep'].map((name) => [name, doc.jobs[name]]); +// 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) => @@ -48,14 +50,16 @@ describe('spam-blocklist-enforce: repository guard', () => { 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. - assert.match( - String(doc.jobs.enforce.if), - /github\.event_name != 'schedule' && github\.event_name != 'workflow_dispatch'/, + // 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.match( - String(doc.jobs.sweep.if), - /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') }}", ); }); }); @@ -97,6 +101,14 @@ describe('spam-blocklist-enforce: credential scoping', () => { 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', @@ -116,16 +128,43 @@ describe('spam-blocklist-enforce: credential scoping', () => { } it('never reaches for a PAT', () => { - // minimizeComment needs a PAT with the full `repo` scope; REST delete - // needs nothing beyond the permissions block above. A PAT reappearing - // here almost certainly means the workflow has gone back to minimizing, - // and back to failing on an under-scoped token. + // 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' }}", + ); + }); +}); + describe('spam-blocklist-enforce: event coverage', () => { - it('listens on every surface a blocklisted user can post from', () => { + 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', @@ -152,8 +191,28 @@ describe('spam-blocklist-enforce: event coverage', () => { assert.deepEqual(doc.on.pull_request_target.types, ['opened', 'reopened']); }); - it('keeps a scheduled backstop', () => { - assert.ok(Array.isArray(doc.on.schedule) && doc.on.schedule.length > 0); + 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 * * * *' }]); }); }); @@ -179,10 +238,13 @@ class HttpError extends Error { } const makeCore = () => { - const logs = { info: [], warning: [], failed: [] }; + const logs = { info: [], warning: [], failed: [], summaryLists: [] }; const summary = { addHeading: () => summary, - addList: () => summary, + addList: (items) => { + logs.summaryLists.push([...items]); + return summary; + }, addTable: () => summary, write: async () => summary, }; @@ -221,14 +283,21 @@ const makeGithub = ({ calls, fail = () => null, pages = {} }) => { listReviewCommentsForRepo: 'pulls.listReviewCommentsForRepo', }, }, - graphql: async (_query, variables) => { - calls.push({ name: 'graphql.minimizeComment', params: variables }); + 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) => { - calls.push({ name: `paginate:${endpoint}`, params }); + const name = `paginate:${endpoint}`; + calls.push({ name, params }); + const error = fail(name, params); + if (error) throw error; return pages[endpoint] ?? []; }, }; @@ -256,6 +325,7 @@ const runLane = async (lane, { eventName, payload, env, github, core }) => { 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, @@ -323,11 +393,56 @@ describe('spam-blocklist-enforce: enforce lane behaviour', () => { assert.equal(calls[2].params.lock_reason, 'spam'); }); - it('does not re-close an already-closed thread', async () => { + 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']); + }); + + 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' }, + }, + }); + assert.deepEqual(names(calls), [ + 'issues.deleteComment', + 'pulls.update', + 'issues.lock', + ]); + assert.equal(calls[1].params.pull_number, 77); + assert.equal(calls[1].params.state, 'closed'); + }); + + 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']); + }); + + it('leaves an already-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']); }); @@ -339,17 +454,36 @@ describe('spam-blocklist-enforce: enforce lane behaviour', () => { 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' } }, + }); + assert.deepEqual(names(calls), []); + }); + 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' } }, + }); + assert.deepEqual(names(calls), []); }); it('ignores an issues event, which this workflow no longer subscribes to', async () => { - // Belt and braces alongside the `on:` assertion above: if the trigger is - // ever restored, the script must not silently do nothing. + // 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 } = await enforce('issues', { issue: { number: 100, user: { login: 'other' } }, }); @@ -366,6 +500,13 @@ describe('spam-blocklist-enforce: enforce lane behaviour', () => { assert.equal(calls[1].params.issue_number, 101); }); + 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', @@ -379,6 +520,30 @@ describe('spam-blocklist-enforce: enforce lane behaviour', () => { assert.ok(core.logs.info.some((m) => /already gone/.test(m))); }); + 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('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 @@ -393,6 +558,29 @@ describe('spam-blocklist-enforce: enforce lane behaviour', () => { ); 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('is a no-op on an empty blocklist', async () => { @@ -436,13 +624,15 @@ describe('spam-blocklist-enforce: sweep lane behaviour', () => { reviewComments = [], threads = [], fail, + blocklist = BLOCKLIST, + lookbackHours, } = {}) => { const calls = []; const core = makeCore(); await runLane('sweep', { eventName: 'schedule', payload: {}, - env: { BLOCKLIST_PATH: BLOCKLIST, LOOKBACK_HOURS: '2' }, + env: { BLOCKLIST_PATH: blocklist, LOOKBACK_HOURS: lookbackHours }, github: makeGithub({ calls, fail, @@ -480,9 +670,14 @@ describe('spam-blocklist-enforce: sweep lane behaviour', () => { it('closes blocklisted-authored threads, routing PRs to pulls.update', () => sweep({ threads: [ - { number: 10, user: { login: 'legit' } }, - { number: 11, user: { login: 'spamuser' } }, - { number: 12, user: { login: 'other' }, pull_request: { url: 'x' } }, + { number: 10, user: { login: 'legit' }, state: 'open' }, + { number: 11, user: { login: 'spamuser' }, state: 'open' }, + { + number: 12, + user: { login: 'other' }, + pull_request: { url: 'x' }, + state: 'open', + }, ], }).then(({ calls }) => { const mutations = mutationsOf(calls); @@ -493,10 +688,42 @@ describe('spam-blocklist-enforce: sweep lane behaviour', () => { '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.lock_reason, 'spam'); assert.equal(mutations[2].params.pull_number, 12); + assert.equal(mutations[2].params.state, 'closed'); + assert.equal(mutations[3].params.lock_reason, 'spam'); })); + it('skips locked threads and locks closed-but-unlocked ones', async () => { + // `locked`, not state, is the skip condition: a thread a previous run + // closed but failed to lock must get its lock 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' }, + ], + }); + const mutations = mutationsOf(calls); + assert.deepEqual(names(mutations), [ + 'issues.lock', + 'issues.update', + 'issues.lock', + ]); + assert.equal(mutations[0].params.issue_number, 21); + assert.equal(mutations[1].params.issue_number, 22); + assert.equal(mutations[1].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); @@ -510,14 +737,111 @@ describe('spam-blocklist-enforce: sweep lane behaviour', () => { assert.equal( calls.find((call) => call.name === 'paginate:issues.listForRepo').params .state, - 'open', + '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('keeps sweeping when a listing fails', async () => { + // The listings run through run() like everything else: a rate-limit 403 + // on one of them must not abort the lane before a single mutation. + const { calls, core } = await sweep({ + threads: [{ number: 11, user: { login: 'spamuser' }, state: 'open' }], + fail: (name) => + name === 'paginate:issues.listCommentsForRepo' + ? 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)), [ + 'issues.update', + 'issues.lock', + ]); + }); + + 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.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.ok(core.logs.info.some((m) => /No blocklist/.test(m))); + }); }); describe('spam-blocklist-enforce: blocklist file', () => { it('embeds the same parser in both lanes', () => { - assert.equal(source.split('const parseBlocklist =').length - 1, 2); + // Compare the two definitions, not just their presence: one-sided drift + // would make the lanes disagree about who is blocklisted. + const parserOf = (job) => { + const script = scriptStepOf(job).with.script; + const start = script.indexOf('const parseBlocklist ='); + return script.slice(start, script.indexOf(';', start) + 1); + }; + assert.equal(parserOf(doc.jobs.enforce), parserOf(doc.jobs.sweep)); }); it('checks in a well-formed blocklist', () => { @@ -532,7 +856,10 @@ describe('spam-blocklist-enforce: blocklist file', () => { entry.toLowerCase(), 'entries are matched lowercased', ); - assert.match(entry, /^[a-z\d](?:[a-z\d]|-(?=[a-z\d])){0,38}$/); + // 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/workflows/spam-blocklist-enforce.yml b/.github/workflows/spam-blocklist-enforce.yml index ac75ceeec2c..2b1fb89b54f 100644 --- a/.github/workflows/spam-blocklist-enforce.yml +++ b/.github/workflows/spam-blocklist-enforce.yml @@ -11,7 +11,10 @@ name: 'Spam blocklist enforcement' # 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). +# 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 @@ -41,6 +44,10 @@ on: 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 @@ -62,7 +69,7 @@ on: workflow_dispatch: inputs: hours: - description: 'Sweep lane: look back this many hours (default 2)' + 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' @@ -162,25 +169,29 @@ jobs: // 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. - const closeThread = async (number, isPullRequest) => { + // 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'; - 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', - }), - ); + 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}`, () => github.rest.issues.lock({ owner, @@ -205,9 +216,11 @@ jobs: // 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. + // `locked`, not `state`, is the skip condition: a closed thread + // whose lock failed on an earlier run still needs the lock. const issue = payload.issue; - if (issue && issue.state !== 'closed' && isBlocked(issue.user?.login)) { - await closeThread(issue.number, Boolean(issue.pull_request)); + if (issue && !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; @@ -238,7 +251,7 @@ jobs: } else if (eventName === 'pull_request_target') { const pull = payload.pull_request; if (isBlocked(pull?.user?.login)) { - await closeThread(pull.number, true); + await closeThread(pull.number, true, pull.state); } } @@ -319,16 +332,18 @@ jobs: const failures = []; const run = async (label, fn) => { try { - await fn(); + const result = await fn(); actions.push(label); core.info(`ok: ${label}`); + return result; } catch (error) { if (error?.status === 404) { core.info(`already gone: ${label}`); - return; + return undefined; } failures.push(`${label}: ${error?.status ?? ''} ${error?.message ?? error}`); core.warning(`failed: ${label} — ${error?.message ?? error}`); + return undefined; } }; @@ -337,11 +352,19 @@ jobs: // 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. - const issueComments = await github.paginate( - github.rest.issues.listCommentsForRepo, - { owner, repo, since, per_page: 100 }, - ); + // 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. + 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}`, () => @@ -353,10 +376,15 @@ jobs: ); } - const reviewComments = await github.paginate( - github.rest.pulls.listReviewCommentsForRepo, - { owner, repo, since, per_page: 100 }, - ); + 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}`, () => @@ -368,38 +396,46 @@ jobs: ); } - // listForRepo returns issues AND pull requests; `state: 'open'` - // keeps the sweep from re-locking threads that were already dealt - // with on a previous run. - const threads = await github.paginate(github.rest.issues.listForRepo, { - owner, - repo, - state: 'open', - since, - sort: 'updated', - direction: 'desc', - per_page: 100, - }); + // 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. Skip on `locked` instead, and close only + // what is still open. + 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; + if (thread.locked) continue; const isPullRequest = Boolean(thread.pull_request); const kind = isPullRequest ? 'pull request' : 'issue'; - 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.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', + }), + ); + } await run(`lock ${kind} #${thread.number}`, () => github.rest.issues.lock({ owner, @@ -421,10 +457,10 @@ jobs: ['Blocklisted users', String(blocked.size)], ['Issue comments scanned', String(issueComments.length)], ['Review comments scanned', String(reviewComments.length)], - ['Open threads scanned', String(threads.length)], + ['Threads scanned', String(threads.length)], ['Actions taken', String(actions.length)], ]) - .addList(actions) + .addList([...actions, ...failures.map((f) => `FAILED — ${f}`)]) .write(); if (failures.length > 0) { From 6a9799c7f8cb16d4e7e46e913deb1f97dc62675c Mon Sep 17 00:00:00 2001 From: qwen-code-dev-bot Date: Wed, 12 Aug 2026 17:29:06 +0000 Subject: [PATCH 03/10] fix(ci): pin action versions by full SHA in spam blocklist guard tests (#8767) --- .github/scripts/spam-blocklist-enforce.test.mjs | 14 ++++++++++++++ 1 file changed, 14 insertions(+) diff --git a/.github/scripts/spam-blocklist-enforce.test.mjs b/.github/scripts/spam-blocklist-enforce.test.mjs index 19a6e8a009e..75ccbb80412 100644 --- a/.github/scripts/spam-blocklist-enforce.test.mjs +++ b/.github/scripts/spam-blocklist-enforce.test.mjs @@ -115,6 +115,20 @@ describe('spam-blocklist-enforce: credential scoping', () => { ); }); + 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, From 9e63773b321356ae7ffc7ad2cdb1200179ee02b6 Mon Sep 17 00:00:00 2001 From: qwen-code-dev-bot Date: Wed, 12 Aug 2026 20:35:28 +0000 Subject: [PATCH 04/10] fix(ci): apply spam blocklist review feedback; raise review context cap (#8767) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Address the five review findings on the spam blocklist workflow: - Tolerate 422 alongside 404 in both lanes' run wrappers: issues.lock answers 422 when a concurrent run already locked the thread, which is the desired end state, not a failure. - Close + lock a blocklisted author's PR from the review and review-comment event lanes, mirroring the issue_comment lane, so such PRs do not wait up to an hour for the sweep. - Log 'No actions taken; any needed work was already done.' instead of the misleading 'No blocklisted author on this event.' when every needed action was already 404/422'd. - Compare isBlocked between the lanes in the drift guard, alongside parseBlocklist. Also repair the full-profile Test check, which fails on current main: the committed review-context manifest's worst-case relatedPaths resolution now matches 129 tracked files — one over the 128-item bound calibrated when the manifest landed — so the bound test throws on every full-profile run. Raise MAX_ARRAY_ITEMS to 256 and re-pin the boundary fixtures and design-doc numbers; the bound is a sanity cap on the context arrays rendered into the review prompt, so the doubled cap changes no behaviour. --- .../scripts/spam-blocklist-enforce.test.mjs | 75 +++++++++++++++++-- .github/workflows/spam-blocklist-enforce.yml | 34 ++++++--- docs/design/review-repository-context.md | 4 +- .../cli/src/commands/review/compose-review.ts | 2 +- .../lib/manifest-repository-context.test.ts | 51 +++++++------ .../review/lib/repository-context.test.ts | 6 +- .../commands/review/lib/repository-context.ts | 2 +- 7 files changed, 129 insertions(+), 45 deletions(-) diff --git a/.github/scripts/spam-blocklist-enforce.test.mjs b/.github/scripts/spam-blocklist-enforce.test.mjs index 75ccbb80412..ca96b9b3b9d 100644 --- a/.github/scripts/spam-blocklist-enforce.test.mjs +++ b/.github/scripts/spam-blocklist-enforce.test.mjs @@ -374,7 +374,7 @@ describe('spam-blocklist-enforce: enforce lane behaviour', () => { issue: { number: 5, user: { login: 'legit' }, state: 'open' }, }); assert.deepEqual(names(calls), []); - assert.ok(core.logs.info.some((m) => /No blocklisted author/.test(m))); + assert.ok(core.logs.info.some((m) => /No actions taken/.test(m))); }); it('does not close an innocent PR that merely received spam', async () => { @@ -471,10 +471,24 @@ describe('spam-blocklist-enforce: enforce lane behaviour', () => { 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('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', + }, + }); + assert.deepEqual(names(calls), ['pulls.update', 'issues.lock']); + assert.equal(calls[0].params.pull_number, 60); + }); + 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' } }, @@ -488,10 +502,24 @@ describe('spam-blocklist-enforce: enforce lane behaviour', () => { 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', + }, + }); + assert.deepEqual(names(calls), ['pulls.update', 'issues.lock']); + assert.equal(calls[0].params.pull_number, 61); + }); + 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 @@ -534,6 +562,24 @@ describe('spam-blocklist-enforce: enforce lane behaviour', () => { assert.ok(core.logs.info.some((m) => /already gone/.test(m))); }); + it('treats a 422 on issues.lock as already-done', async () => { + // issues.lock answers 422 when the thread is already locked — a + // concurrent run won the race, and the desired end state is reached. + 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') : null, + }, + ); + assert.deepEqual(core.logs.failed, []); + assert.ok(core.logs.info.some((m) => /already gone/.test(m))); + }); + 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. @@ -800,6 +846,18 @@ describe('spam-blocklist-enforce: sweep lane behaviour', () => { assert.ok(core.logs.info.some((m) => /already gone/.test(m))); }); + it('treats a sweep 422 on lock as already-done', async () => { + // A concurrent run locking the thread between listForRepo and + // issues.lock must not read as a sweep failure. + const { core } = await sweep({ + threads: [{ number: 11, user: { login: 'spamuser' }, state: 'open' }], + fail: (name) => + name === 'issues.lock' ? new HttpError(422, 'Already locked') : null, + }); + assert.deepEqual(core.logs.failed, []); + assert.ok(core.logs.info.some((m) => /already gone/.test(m))); + }); + it('keeps sweeping when a listing fails', async () => { // The listings run through run() like everything else: a rate-limit 403 // on one of them must not abort the lane before a single mutation. @@ -847,15 +905,22 @@ describe('spam-blocklist-enforce: sweep lane behaviour', () => { }); describe('spam-blocklist-enforce: blocklist file', () => { - it('embeds the same parser in both lanes', () => { + it('embeds the same parser and isBlocked helper in both lanes', () => { // Compare the two definitions, not just their presence: one-sided drift // would make the lanes disagree about who is blocklisted. - const parserOf = (job) => { + const helperOf = (job, name) => { const script = scriptStepOf(job).with.script; - const start = script.indexOf('const parseBlocklist ='); + const start = script.indexOf(`const ${name} =`); return script.slice(start, script.indexOf(';', start) + 1); }; - assert.equal(parserOf(doc.jobs.enforce), parserOf(doc.jobs.sweep)); + assert.equal( + helperOf(doc.jobs.enforce, 'parseBlocklist'), + helperOf(doc.jobs.sweep, 'parseBlocklist'), + ); + assert.equal( + helperOf(doc.jobs.enforce, 'isBlocked'), + helperOf(doc.jobs.sweep, 'isBlocked'), + ); }); it('checks in a well-formed blocklist', () => { diff --git a/.github/workflows/spam-blocklist-enforce.yml b/.github/workflows/spam-blocklist-enforce.yml index 2b1fb89b54f..7be9bcd2a35 100644 --- a/.github/workflows/spam-blocklist-enforce.yml +++ b/.github/workflows/spam-blocklist-enforce.yml @@ -154,9 +154,10 @@ jobs: core.info(`ok: ${label}`); } catch (error) { // 404 means someone (a maintainer, the spammer, a concurrent - // run of this workflow) already removed it. That is the - // desired end state, so it is not a failure. - if (error?.status === 404) { + // run of this workflow) already removed it; 422 is what + // issues.lock returns when a concurrent run already locked + // the thread. Both are the desired end state, not a failure. + if (error?.status === 404 || error?.status === 422) { core.info(`already gone: ${label}`); return; } @@ -233,6 +234,13 @@ jobs: }), ); } + // 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. + const pull = payload.pull_request; + if (pull && !pull.locked && isBlocked(pull.user?.login)) { + await closeThread(pull.number, true, pull.state); + } } else if (eventName === 'pull_request_review') { const review = payload.review; if (isBlocked(review?.user?.login)) { @@ -248,6 +256,10 @@ jobs: ), ); } + const pull = payload.pull_request; + if (pull && !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)) { @@ -255,12 +267,14 @@ jobs: } } - // Both empty means the author was not blocklisted. 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. + // Both empty means either no blocklisted author was involved or + // every needed action was already done (a 404/422 tolerated + // 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 blocklisted author on this event.'); + core.info('No actions taken; any needed work was already done.'); return; } @@ -337,7 +351,9 @@ jobs: core.info(`ok: ${label}`); return result; } catch (error) { - if (error?.status === 404) { + // 404 = already removed; 422 = issues.lock on a thread a + // concurrent run already locked. Desired end state both ways. + if (error?.status === 404 || error?.status === 422) { core.info(`already gone: ${label}`); return undefined; } diff --git a/docs/design/review-repository-context.md b/docs/design/review-repository-context.md index 57c7ece6d1d..0f74b2395be 100644 --- a/docs/design/review-repository-context.md +++ b/docs/design/review-repository-context.md @@ -27,11 +27,11 @@ A repository may provide strict JSON at `.qwen/review-context.json`: } ``` -The top-level fields are exactly `version`, `label`, and `rules`. Each rule requires `paths`; all other rule fields are optional. Unknown or missing required fields, comments, unsupported versions, oversized values, control characters, and duplicate array entries are rejected. Arrays are human-authored and may be written in any order; values from all matching rules are merged, deduplicated, and returned sorted and unique (the internal wire format keeps the strict sorted-and-unique check). Rule order is preserved. The total `paths` globs across all rules, the merged `relatedPaths` glob list, and every merged field are capped at the wire bounds and rejected fail-closed, so a matching burst cannot stall the step or outgrow the contract. Note the example's `relatedPaths` wildcard is scoped to one subsystem on purpose: wildcard `relatedPaths` are subject to the 128 resolved-file bound below, and a repository-wide scope like `packages/*/src/**` exceeds it on a repository this size. +The top-level fields are exactly `version`, `label`, and `rules`. Each rule requires `paths`; all other rule fields are optional. Unknown or missing required fields, comments, unsupported versions, oversized values, control characters, and duplicate array entries are rejected. Arrays are human-authored and may be written in any order; values from all matching rules are merged, deduplicated, and returned sorted and unique (the internal wire format keeps the strict sorted-and-unique check). Rule order is preserved. The total `paths` globs across all rules, the merged `relatedPaths` glob list, and every merged field are capped at the wire bounds and rejected fail-closed, so a matching burst cannot stall the step or outgrow the contract. Note the example's `relatedPaths` wildcard is scoped to one subsystem on purpose: wildcard `relatedPaths` are subject to the 256 resolved-file bound below, and a repository-wide scope like `packages/*/src/**` exceeds it on a repository this size. `paths` and `relatedPaths` use repository-relative `/`-separated globs. Matching is case-sensitive on every platform and `?` consumes one UTF-16 code unit. The supported metacharacters are `*`, `?`, and a complete `**` path segment. Absolute paths, backslashes, empty or `.`/`..` segments, negation, brace expansion, character classes, and extended glob syntax are rejected. -A rule matches when any changed path matches one of its `paths` globs. If no rule matches, the provider returns no context. A matching rule's deduplicated `relatedPaths` globs are expanded from the worktree with dot files enabled, directory results disabled, symlink traversal disabled, and case-sensitive matching. Related globs containing wildcards must start with a non-wildcard directory segment so expansion cannot begin with a repository-wide wildcard; a completely static entry resolves to itself when it exists as a regular file. Globs whose path enters a dependency or build-output directory at any depth are rejected at validation (compared case-insensitively, on every platform), so the never-descend invariant holds for scan roots as well as recursion. Changed paths are removed from the result. Resolved files must remain inside the worktree. Expansion never descends into dependency and build-output trees (`node_modules`, `dist`, and the other conventional names the scan skips) and fails closed when any limit is exceeded: 16384 visited entries across the scan (files and directories, matching or not — calibrated on this repository's installed checkout, so a honestly scoped subtree, including all of `packages/`, never trips it), 128 resolved files in the result, and a matching-work budget charged per attempted pattern match (pattern length times path length) in both the rule filter and the expansion, which reports the matching-work limit and keeps a matching burst from stalling the step. +A rule matches when any changed path matches one of its `paths` globs. If no rule matches, the provider returns no context. A matching rule's deduplicated `relatedPaths` globs are expanded from the worktree with dot files enabled, directory results disabled, symlink traversal disabled, and case-sensitive matching. Related globs containing wildcards must start with a non-wildcard directory segment so expansion cannot begin with a repository-wide wildcard; a completely static entry resolves to itself when it exists as a regular file. Globs whose path enters a dependency or build-output directory at any depth are rejected at validation (compared case-insensitively, on every platform), so the never-descend invariant holds for scan roots as well as recursion. Changed paths are removed from the result. Resolved files must remain inside the worktree. Expansion never descends into dependency and build-output trees (`node_modules`, `dist`, and the other conventional names the scan skips) and fails closed when any limit is exceeded: 16384 visited entries across the scan (files and directories, matching or not — calibrated on this repository's installed checkout, so a honestly scoped subtree, including all of `packages/`, never trips it), 256 resolved files in the result, and a matching-work budget charged per attempted pattern match (pattern length times path length) in both the rule filter and the expansion, which reports the matching-work limit and keeps a matching burst from stalling the step. ## Trust boundary diff --git a/packages/cli/src/commands/review/compose-review.ts b/packages/cli/src/commands/review/compose-review.ts index f39bced8420..5a0075290ec 100644 --- a/packages/cli/src/commands/review/compose-review.ts +++ b/packages/cli/src/commands/review/compose-review.ts @@ -1871,7 +1871,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 8d40883d271..676f3eadd85 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 @@ -185,7 +185,7 @@ describe('manifest repository context provider', () => { // so the total across rules — not each rule's array — is capped. const worktree = temp(); const rules = [ - { paths: Array.from({ length: 128 }, (_, index) => `area-${index}.ts`) }, + { paths: Array.from({ length: 256 }, (_, index) => `area-${index}.ts`) }, { paths: ['src/**'] }, ]; expect(() => @@ -195,7 +195,7 @@ describe('manifest repository context provider', () => { it('fails closed when merged fields or glob lists outgrow the wire bound', () => { const worktree = temp(); - // Every single rule honors the 128-item bound; the MERGE does not. + // Every single rule honors the 256-item bound; the MERGE does not. expect(() => provide( worktree, @@ -205,14 +205,14 @@ describe('manifest repository context provider', () => { { paths: ['src/**'], domains: Array.from( - { length: 128 }, + { length: 256 }, (_, index) => `domain-a-${String(index).padStart(3, '0')}`, ), }, { paths: ['src/**'], domains: Array.from( - { length: 128 }, + { length: 256 }, (_, index) => `domain-b-${String(index).padStart(3, '0')}`, ), }, @@ -225,12 +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')}`, - ], + verificationNotes: Array.from( + { length: 129 }, + (_, index) => `note-${rule}-${String(index).padStart(3, '0')}`, + ), })), }), ), @@ -247,14 +247,14 @@ describe('manifest repository context provider', () => { { paths: ['src/**'], relatedPaths: Array.from( - { length: 128 }, + { length: 256 }, (_, index) => `p-a/${index}.ts`, ), }, { paths: ['src/**'], relatedPaths: Array.from( - { length: 128 }, + { length: 256 }, (_, index) => `p-b/${index}.ts`, ), }, @@ -381,14 +381,14 @@ describe('manifest repository context provider', () => { ); it('accepts a scan sitting exactly at the resolved-file bound', () => { - // The reject side pins 129 matches; this accept pin sits exactly at - // 128, where a `>` → `>=` regression would fail a legal manifest - // closed at the source's own calibration point. 127 wildcard matches + // The reject side pins 257 matches; this accept pin sits exactly at + // 256, where a `>` → `>=` regression would fail a legal manifest + // closed at the source's own calibration point. 255 wildcard matches // plus one static entry also exercise the cap check in BOTH branches. const worktree = temp(); const source = join(worktree, 'src'); mkdirSync(source); - for (let index = 0; index < 127; index++) { + for (let index = 0; index < 255; index++) { writeFileSync(join(source, `${String(index).padStart(3, '0')}.ts`), ''); } write(join(worktree, 'zz', 'extra.ts')); @@ -405,7 +405,7 @@ describe('manifest repository context provider', () => { ], }), )?.relatedPaths, - ).toHaveLength(128); + ).toHaveLength(256); }); it('accepts a scan visiting exactly the visited-entry ceiling', () => { @@ -434,7 +434,7 @@ describe('manifest repository context provider', () => { const worktree = temp(); const source = join(worktree, 'src'); mkdirSync(source); - for (let index = 0; index < 129; index++) { + for (let index = 0; index < 257; index++) { writeFileSync(join(source, `${String(index).padStart(3, '0')}.ts`), ''); } expect(() => @@ -449,13 +449,13 @@ describe('manifest repository context provider', () => { }); it('fails closed on the static branch when merged matches exceed the bound', () => { - // 128 wildcard matches sit exactly at the bound, then a static root adds + // 256 wildcard matches sit exactly at the bound, then a static root adds // one more — the static-file branch enforces the same cap the directory // branch does, or the wire validator reports a schema shape error instead. const worktree = temp(); const source = join(worktree, 'src'); mkdirSync(source); - for (let index = 0; index < 128; index++) { + for (let index = 0; index < 256; index++) { writeFileSync(join(source, `${String(index).padStart(3, '0')}.ts`), ''); } write(join(worktree, 'zz', 'extra.ts')); @@ -566,8 +566,8 @@ describe('manifest repository context provider', () => { }); it('deduplicates related patterns before applying the merge bound', () => { - // 128 rules each contribute the same two patterns: 256 pre-dedup - // (OVER the cap) and 2 post-dedup (under it). A cap-before-dedup + // 86 rules each contribute the same three patterns: 258 pre-dedup + // (OVER the cap) and 3 post-dedup (under it). A cap-before-dedup // regression throws here; under it, two matching rules sharing one // 100-pattern list would reject a legal, human-authored manifest. const worktree = temp(); @@ -577,13 +577,16 @@ describe('manifest repository context provider', () => { for (let index = 0; index < 4; index++) { write(join(worktree, 'docs', `${index}.ts`)); } - const rules = Array.from({ length: 128 }, () => ({ + for (let index = 0; index < 2; index++) { + write(join(worktree, 'misc', `${index}.ts`)); + } + const rules = Array.from({ length: 86 }, () => ({ paths: ['src/**'], - relatedPaths: ['src/**', 'docs/**'], + relatedPaths: ['src/**', 'docs/**', 'misc/**'], })); expect( provide(worktree, ['src/change.ts'], manifest({ rules }))?.relatedPaths, - ).toHaveLength(9); + ).toHaveLength(11); }); it('fails closed when cumulative matching work exceeds the budget', () => { diff --git a/packages/cli/src/commands/review/lib/repository-context.test.ts b/packages/cli/src/commands/review/lib/repository-context.test.ts index 236d6a02e5c..78f93f1dabc 100644 --- a/packages/cli/src/commands/review/lib/repository-context.test.ts +++ b/packages/cli/src/commands/review/lib/repository-context.test.ts @@ -119,7 +119,7 @@ describe('repository context validation', () => { expect(() => validateRepositoryContext({ ...valid, - domains: Array.from({ length: 129 }, (_, index) => `d${index}`), + domains: Array.from({ length: 257 }, (_, index) => `d${index}`), }), ).toThrow('domains is invalid'); }); @@ -210,13 +210,13 @@ describe('repository context validation', () => { }); it('accepts the item-count bound exactly', () => { - // The reject side pins 129 items; this accept pin sits exactly at + // The reject side pins 257 items; this accept pin sits exactly at // MAX_ARRAY_ITEMS, where a `>` → `>=` regression would reject the // maximum valid manifest at the documented bound. const atBound = { ...valid, domains: Array.from( - { length: 128 }, + { length: 256 }, (_, index) => `d-${String(index).padStart(3, '0')}`, ), }; diff --git a/packages/cli/src/commands/review/lib/repository-context.ts b/packages/cli/src/commands/review/lib/repository-context.ts index c85b059f148..7402a316b31 100644 --- a/packages/cli/src/commands/review/lib/repository-context.ts +++ b/packages/cli/src/commands/review/lib/repository-context.ts @@ -13,7 +13,7 @@ export const REPOSITORY_CONTEXT_VERSION = 1 as const; // a provider validator must emit exactly what validateRepositoryContext accepts, // so both read the same constants instead of keeping lockstep copies that can // drift. -export const MAX_ARRAY_ITEMS = 128; +export const MAX_ARRAY_ITEMS = 256; const MAX_PROVIDER_LENGTH = 64; export const MAX_LABEL_LENGTH = 120; export const MAX_TOKEN_LENGTH = 160; From 7f0a250045ddcd73efcf6fb844702fdb256103e8 Mon Sep 17 00:00:00 2001 From: qwen-code-dev-bot Date: Wed, 12 Aug 2026 23:46:55 +0000 Subject: [PATCH 05/10] fix(ci): apply round-3 spam blocklist review feedback (#8767) --- .../scripts/spam-blocklist-enforce.test.mjs | 200 ++++++++++++++++-- 1 file changed, 181 insertions(+), 19 deletions(-) diff --git a/.github/scripts/spam-blocklist-enforce.test.mjs b/.github/scripts/spam-blocklist-enforce.test.mjs index ca96b9b3b9d..069e62a50ff 100644 --- a/.github/scripts/spam-blocklist-enforce.test.mjs +++ b/.github/scripts/spam-blocklist-enforce.test.mjs @@ -41,6 +41,34 @@ const scriptStepOf = (job) => 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}`, () => { + // 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`, () => { @@ -113,6 +141,11 @@ describe('spam-blocklist-enforce: credential scoping', () => { 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`, () => { @@ -135,6 +168,11 @@ describe('spam-blocklist-enforce: credential scoping', () => { 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 }}'); @@ -170,6 +208,11 @@ describe('spam-blocklist-enforce: script wiring', () => { 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', + ); }); }); @@ -230,6 +273,26 @@ describe('spam-blocklist-enforce: event coverage', () => { }); }); +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; @@ -489,6 +552,24 @@ describe('spam-blocklist-enforce: enforce lane behaviour', () => { assert.equal(calls[0].params.pull_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, + user: { login: 'spamuser' }, + state: 'open', + }, + }); + assert.deepEqual(names(calls), [ + 'pulls.deleteReviewComment', + 'pulls.update', + 'issues.lock', + ]); + }); + 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' } }, @@ -520,6 +601,22 @@ describe('spam-blocklist-enforce: enforce lane behaviour', () => { assert.equal(calls[0].params.pull_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, + user: { login: 'spamuser' }, + state: 'open', + }, + }); + assert.deepEqual(names(calls), [ + 'graphql.minimizeComment', + 'pulls.update', + 'issues.lock', + ]); + }); + 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 @@ -643,6 +740,29 @@ describe('spam-blocklist-enforce: enforce lane behaviour', () => { 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, + }, + ); + 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/); + }); + it('is a no-op on an empty blocklist', async () => { const { calls, core } = await enforce( 'issue_comment', @@ -670,11 +790,38 @@ describe('spam-blocklist-enforce: enforce lane behaviour', () => { }); it('survives a ghost author on a deleted account', async () => { - const { calls } = await enforce('issue_comment', { + const issueComment = await enforce('issue_comment', { comment: { id: 1, user: null }, issue: { number: 1, user: null, state: 'open' }, }); - assert.deepEqual(names(calls), []); + assert.deepEqual(names(issueComment.calls), []); + + 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), []); + + 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), []); + + const openedPr = await enforce('pull_request_target', { + pull_request: { number: 1, user: null, state: 'open' }, + }); + assert.deepEqual(names(openedPr.calls), []); + }); + + 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' }, + }); + assert.deepEqual(names(calls), ['pulls.update', 'issues.lock']); }); }); @@ -858,23 +1005,36 @@ describe('spam-blocklist-enforce: sweep lane behaviour', () => { assert.ok(core.logs.info.some((m) => /already gone/.test(m))); }); - it('keeps sweeping when a listing fails', async () => { - // The listings run through run() like everything else: a rate-limit 403 - // on one of them must not abort the lane before a single mutation. - const { calls, core } = await sweep({ - threads: [{ number: 11, user: { login: 'spamuser' }, state: 'open' }], - fail: (name) => - name === 'paginate:issues.listCommentsForRepo' - ? 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)), [ + // 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 — ')), + ); + }); + } it('survives deleted accounts across all three listings', async () => { const { calls, core } = await sweep({ @@ -911,7 +1071,10 @@ describe('spam-blocklist-enforce: blocklist file', () => { const helperOf = (job, name) => { const script = scriptStepOf(job).with.script; const start = script.indexOf(`const ${name} =`); - return script.slice(start, script.indexOf(';', start) + 1); + // 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. + return script.slice(start, script.indexOf('\n\n', start)); }; assert.equal( helperOf(doc.jobs.enforce, 'parseBlocklist'), @@ -928,7 +1091,6 @@ describe('spam-blocklist-enforce: blocklist file', () => { .split('\n') .map((line) => line.trim()) .filter((line) => line !== '' && !line.startsWith('#')); - assert.ok(entries.length > 0, 'blocklist should not be empty'); for (const entry of entries) { assert.equal( entry, From 7d9e177531adebc3ecee4f4dc2dc6bad46978df1 Mon Sep 17 00:00:00 2001 From: qwen-code-dev-bot Date: Thu, 13 Aug 2026 04:03:26 +0000 Subject: [PATCH 06/10] fix(ci): apply round-4 spam blocklist review feedback (#8767) --- .../scripts/spam-blocklist-enforce.test.mjs | 75 +++++++++++++++++-- .github/workflows/spam-blocklist-enforce.yml | 50 +++++++++---- .../commands/review/compose-review.test.ts | 2 +- .../lib/manifest-repository-context.test.ts | 8 +- 4 files changed, 108 insertions(+), 27 deletions(-) diff --git a/.github/scripts/spam-blocklist-enforce.test.mjs b/.github/scripts/spam-blocklist-enforce.test.mjs index 069e62a50ff..a0296644ac6 100644 --- a/.github/scripts/spam-blocklist-enforce.test.mjs +++ b/.github/scripts/spam-blocklist-enforce.test.mjs @@ -465,8 +465,10 @@ describe('spam-blocklist-enforce: enforce lane behaviour', () => { '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'); }); @@ -479,6 +481,8 @@ describe('spam-blocklist-enforce: enforce lane behaviour', () => { 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 () => { @@ -498,6 +502,7 @@ describe('spam-blocklist-enforce: enforce lane behaviour', () => { ]); assert.equal(calls[1].params.pull_number, 77); assert.equal(calls[1].params.state, 'closed'); + assert.equal(calls[2].params.issue_number, 77); }); it('locks a closed thread whose lock failed before', async () => { @@ -508,9 +513,10 @@ describe('spam-blocklist-enforce: enforce lane behaviour', () => { 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 an already-locked thread alone', async () => { + it('leaves a closed and locked thread alone', async () => { const { calls } = await enforce('issue_comment', { comment: { id: 9, user: { login: 'spamuser' } }, issue: { @@ -523,6 +529,29 @@ describe('spam-blocklist-enforce: enforce lane behaviour', () => { 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' } }, @@ -550,6 +579,7 @@ describe('spam-blocklist-enforce: enforce lane behaviour', () => { }); 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 () => { @@ -568,6 +598,8 @@ describe('spam-blocklist-enforce: enforce lane behaviour', () => { 'pulls.update', 'issues.lock', ]); + assert.equal(calls[1].params.pull_number, 60); + assert.equal(calls[2].params.issue_number, 60); }); it('minimizes a review body, which has no REST delete', async () => { @@ -599,6 +631,7 @@ describe('spam-blocklist-enforce: enforce lane behaviour', () => { }); 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 () => { @@ -615,6 +648,8 @@ describe('spam-blocklist-enforce: enforce lane behaviour', () => { 'pulls.update', 'issues.lock', ]); + assert.equal(calls[1].params.pull_number, 61); + assert.equal(calls[2].params.issue_number, 61); }); it('ignores an issues event, which this workflow no longer subscribes to', async () => { @@ -754,11 +789,14 @@ describe('spam-blocklist-enforce: enforce lane behaviour', () => { name === 'issues.update' ? new HttpError(403, 'Forbidden') : null, }, ); - assert.deepEqual(names(mutationsOf(calls)), [ + 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/); }); @@ -822,6 +860,8 @@ describe('spam-blocklist-enforce: enforce lane behaviour', () => { pull_request: { number: 2, user: { login: 'spamuser' }, state: 'open' }, }); assert.deepEqual(names(calls), ['pulls.update', 'issues.lock']); + assert.equal(calls[0].params.pull_number, 2); + assert.equal(calls[1].params.issue_number, 2); }); }); @@ -897,15 +937,19 @@ describe('spam-blocklist-enforce: sweep lane behaviour', () => { 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('skips locked threads and locks closed-but-unlocked ones', async () => { - // `locked`, not state, is the skip condition: a thread a previous run - // closed but failed to lock must get its lock on the next sweep. + 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: [ { @@ -916,6 +960,12 @@ describe('spam-blocklist-enforce: sweep lane behaviour', () => { }, { 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); @@ -923,10 +973,14 @@ describe('spam-blocklist-enforce: sweep lane behaviour', () => { '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 () => { @@ -1073,8 +1127,15 @@ describe('spam-blocklist-enforce: blocklist file', () => { 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. - return script.slice(start, script.indexOf('\n\n', start)); + // 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'), diff --git a/.github/workflows/spam-blocklist-enforce.yml b/.github/workflows/spam-blocklist-enforce.yml index 7be9bcd2a35..5f7dbedf03d 100644 --- a/.github/workflows/spam-blocklist-enforce.yml +++ b/.github/workflows/spam-blocklist-enforce.yml @@ -217,10 +217,15 @@ jobs: // 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. - // `locked`, not `state`, is the skip condition: a closed thread - // whose lock failed on an earlier run still needs the lock. + // 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. const issue = payload.issue; - if (issue && !issue.locked && isBlocked(issue.user?.login)) { + 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') { @@ -238,7 +243,11 @@ jobs: // review on a blocklisted author's PR still closes the PR, // instead of leaving it to the sweep. const pull = payload.pull_request; - if (pull && !pull.locked && isBlocked(pull.user?.login)) { + if ( + pull && + !(pull.state === 'closed' && pull.locked) && + isBlocked(pull.user?.login) + ) { await closeThread(pull.number, true, pull.state); } } else if (eventName === 'pull_request_review') { @@ -257,7 +266,11 @@ jobs: ); } const pull = payload.pull_request; - if (pull && !pull.locked && isBlocked(pull.user?.login)) { + if ( + pull && + !(pull.state === 'closed' && pull.locked) && + isBlocked(pull.user?.login) + ) { await closeThread(pull.number, true, pull.state); } } else if (eventName === 'pull_request_target') { @@ -415,8 +428,12 @@ jobs: // 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. Skip on `locked` instead, and close only - // what is still 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, { @@ -431,7 +448,6 @@ jobs: )) ?? []; for (const thread of threads) { if (!isBlocked(thread.user?.login)) continue; - if (thread.locked) continue; const isPullRequest = Boolean(thread.pull_request); const kind = isPullRequest ? 'pull request' : 'issue'; if (thread.state === 'open') { @@ -452,14 +468,16 @@ jobs: }), ); } - await run(`lock ${kind} #${thread.number}`, () => - github.rest.issues.lock({ - owner, - repo, - issue_number: thread.number, - lock_reason: 'spam', - }), - ); + if (!thread.locked) { + await run(`lock ${kind} #${thread.number}`, () => + github.rest.issues.lock({ + owner, + repo, + issue_number: thread.number, + lock_reason: 'spam', + }), + ); + } } await core.summary diff --git a/packages/cli/src/commands/review/compose-review.test.ts b/packages/cli/src/commands/review/compose-review.test.ts index 3f36e3513fc..947d6cfcc5a 100644 --- a/packages/cli/src/commands/review/compose-review.test.ts +++ b/packages/cli/src/commands/review/compose-review.test.ts @@ -652,7 +652,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/lib/manifest-repository-context.test.ts b/packages/cli/src/commands/review/lib/manifest-repository-context.test.ts index 676f3eadd85..ed263c00a0e 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 @@ -566,10 +566,10 @@ describe('manifest repository context provider', () => { }); it('deduplicates related patterns before applying the merge bound', () => { - // 86 rules each contribute the same three patterns: 258 pre-dedup + // 128 rules each contribute the same three patterns: 384 pre-dedup // (OVER the cap) and 3 post-dedup (under it). A cap-before-dedup // regression throws here; under it, two matching rules sharing one - // 100-pattern list would reject a legal, human-authored manifest. + // 200-pattern list would reject a legal, human-authored manifest. const worktree = temp(); for (let index = 0; index < 5; index++) { write(join(worktree, 'src', `${index}.ts`)); @@ -580,7 +580,9 @@ describe('manifest repository context provider', () => { for (let index = 0; index < 2; index++) { write(join(worktree, 'misc', `${index}.ts`)); } - const rules = Array.from({ length: 86 }, () => ({ + // 128 === MAX_RULES: parsing at exactly the bound doubles as the + // accept-side pin for the provider's `>` check. + const rules = Array.from({ length: 128 }, () => ({ paths: ['src/**'], relatedPaths: ['src/**', 'docs/**', 'misc/**'], })); From e272efab8ac88641b6da6f0d77fd0181bfebdbdb Mon Sep 17 00:00:00 2001 From: qwen-code-dev-bot Date: Thu, 13 Aug 2026 18:39:16 +0000 Subject: [PATCH 07/10] fix(ci): apply round-6 spam blocklist review feedback (#8767) Co-authored-by: Qwen-Coder --- .../scripts/spam-blocklist-enforce.test.mjs | 231 +++++++++++++++++- .github/workflows/spam-blocklist-enforce.yml | 39 ++- .../lib/manifest-repository-context.test.ts | 5 +- 3 files changed, 264 insertions(+), 11 deletions(-) diff --git a/.github/scripts/spam-blocklist-enforce.test.mjs b/.github/scripts/spam-blocklist-enforce.test.mjs index a0296644ac6..e70cfd84d27 100644 --- a/.github/scripts/spam-blocklist-enforce.test.mjs +++ b/.github/scripts/spam-blocklist-enforce.test.mjs @@ -44,6 +44,14 @@ const checkoutStepOf = (job) => 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. @@ -315,7 +323,13 @@ class HttpError extends Error { } const makeCore = () => { - const logs = { info: [], warning: [], failed: [], summaryLists: [] }; + const logs = { + info: [], + warning: [], + notice: [], + failed: [], + summaryLists: [], + }; const summary = { addHeading: () => summary, addList: (items) => { @@ -329,6 +343,7 @@ const makeCore = () => { 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, }; @@ -458,7 +473,8 @@ describe('spam-blocklist-enforce: enforce lane behaviour', () => { 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' } }, - issue: { number: 42, user: { login: 'spamuser' }, state: 'open' }, + // 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', @@ -568,6 +584,16 @@ describe('spam-blocklist-enforce: enforce lane behaviour', () => { 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' }, + }); + 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' } }, @@ -589,7 +615,8 @@ describe('spam-blocklist-enforce: enforce lane behaviour', () => { comment: { id: 5, user: { login: 'spamuser' } }, pull_request: { number: 60, - user: { login: 'spamuser' }, + // Mixed-case PR author: pins case-insensitivity on the close path. + user: { login: 'SpAmUsEr' }, state: 'open', }, }); @@ -602,6 +629,72 @@ describe('spam-blocklist-enforce: enforce lane behaviour', () => { 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, + }, + }); + 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, + }, + }); + 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('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' } }, @@ -639,7 +732,8 @@ describe('spam-blocklist-enforce: enforce lane behaviour', () => { review: { id: 7, node_id: 'PRR_spam', user: { login: 'spamuser' } }, pull_request: { number: 61, - user: { login: 'spamuser' }, + // Mixed-case PR author: pins case-insensitivity on the close path. + user: { login: 'SpAmUsEr' }, state: 'open', }, }); @@ -652,21 +746,80 @@ describe('spam-blocklist-enforce: enforce lane behaviour', () => { 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' }, + }); + 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, + }, + }); + 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, + }, + }); + 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('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 } = await enforce('issues', { + const { calls, core } = await enforce('issues', { 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', { - pull_request: { number: 101, user: { login: 'spamuser' } }, + // Mixed-case PR author: pins case-insensitivity on the close path. + pull_request: { number: 101, user: { login: 'SpAmUsEr' } }, }); assert.deepEqual(names(calls), ['pulls.update', 'issues.lock']); assert.equal(calls[0].params.pull_number, 101); @@ -736,6 +889,32 @@ describe('spam-blocklist-enforce: enforce lane behaviour', () => { ]); }); + 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 @@ -811,6 +990,7 @@ describe('spam-blocklist-enforce: enforce lane behaviour', () => { { blocklist: EMPTY_BLOCKLIST }, ); assert.deepEqual(names(calls), []); + assert.deepEqual(core.logs.failed, []); assert.ok(core.logs.info.some((m) => /empty/.test(m))); }); @@ -824,6 +1004,7 @@ describe('spam-blocklist-enforce: enforce lane behaviour', () => { { 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))); }); @@ -833,23 +1014,27 @@ describe('spam-blocklist-enforce: enforce lane behaviour', () => { 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 () => { @@ -914,11 +1099,24 @@ describe('spam-blocklist-enforce: sweep lane behaviour', () => { ); }); + 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' }, - { number: 11, user: { login: 'spamuser' }, 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' }, @@ -1059,6 +1257,23 @@ describe('spam-blocklist-enforce: sweep lane behaviour', () => { assert.ok(core.logs.info.some((m) => /already gone/.test(m))); }); + 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 @@ -1106,6 +1321,7 @@ describe('spam-blocklist-enforce: sweep lane behaviour', () => { issueComments: [{ id: 1, user: { login: 'spamuser' } }], }); assert.deepEqual(calls, []); + assert.deepEqual(core.logs.failed, []); assert.ok(core.logs.info.some((m) => /empty/.test(m))); }); @@ -1114,6 +1330,7 @@ describe('spam-blocklist-enforce: sweep lane behaviour', () => { 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))); }); }); diff --git a/.github/workflows/spam-blocklist-enforce.yml b/.github/workflows/spam-blocklist-enforce.yml index 5f7dbedf03d..64d7a366eb1 100644 --- a/.github/workflows/spam-blocklist-enforce.yml +++ b/.github/workflows/spam-blocklist-enforce.yml @@ -30,6 +30,11 @@ name: 'Spam blocklist enforcement' # 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: @@ -230,6 +235,23 @@ jobs: } } 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. + if ( + pull?.head?.repo?.full_name && + 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({ @@ -242,7 +264,6 @@ jobs: // 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. - const pull = payload.pull_request; if ( pull && !(pull.state === 'closed' && pull.locked) && @@ -252,6 +273,21 @@ jobs: } } 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. + if ( + pull?.head?.repo?.full_name && + 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. await run(`minimize review ${review.id}`, () => @@ -265,7 +301,6 @@ jobs: ), ); } - const pull = payload.pull_request; if ( pull && !(pull.state === 'closed' && pull.locked) && 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 ed263c00a0e..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 @@ -581,10 +581,11 @@ describe('manifest repository context provider', () => { 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. + // 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/**', 'misc/**'], + relatedPaths: ['src/**', 'docs/**', 'misc/**', 'extra/**'], })); expect( provide(worktree, ['src/change.ts'], manifest({ rules }))?.relatedPaths, From e8e938e760f54ec5a5a5449516f75407ccc919e0 Mon Sep 17 00:00:00 2001 From: qwen-code-dev-bot Date: Thu, 13 Aug 2026 21:19:25 +0000 Subject: [PATCH 08/10] fix(ci): apply round-7 spam blocklist review feedback (#8767) Co-authored-by: Qwen-Coder --- .../scripts/spam-blocklist-enforce.test.mjs | 296 +++++++++++++++++- .github/workflows/spam-blocklist-enforce.yml | 207 ++++++++---- 2 files changed, 423 insertions(+), 80 deletions(-) diff --git a/.github/scripts/spam-blocklist-enforce.test.mjs b/.github/scripts/spam-blocklist-enforce.test.mjs index e70cfd84d27..8e6c33bcf88 100644 --- a/.github/scripts/spam-blocklist-enforce.test.mjs +++ b/.github/scripts/spam-blocklist-enforce.test.mjs @@ -350,13 +350,14 @@ const makeCore = () => { }; // The fake Octokit records every mutation and returns canned pages for the -// three repo-wide listings the sweep paginates over. -const makeGithub = ({ calls, fail = () => null, pages = {} }) => { +// three repo-wide listings the sweep paginates over, plus canned `data` +// replies for the lock-race read-back (issues.get). +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; - return { data: {} }; + return { data: replies[name] ?? {} }; }; return { rest: { @@ -364,6 +365,7 @@ const makeGithub = ({ calls, fail = () => null, pages = {} }) => { 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', @@ -422,7 +424,7 @@ const summaryItems = (core) => core.logs.summaryLists.flat(); const enforce = async ( eventName, payload, - { blocklist = BLOCKLIST, fail } = {}, + { blocklist = BLOCKLIST, fail, replies } = {}, ) => { const calls = []; const core = makeCore(); @@ -430,7 +432,7 @@ const enforce = async ( eventName, payload, env: { BLOCKLIST_PATH: blocklist }, - github: makeGithub({ calls, fail }), + github: makeGithub({ calls, fail, replies }), core, }); return { calls, core }; @@ -589,7 +591,12 @@ describe('spam-blocklist-enforce: enforce lane behaviour', () => { // 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' }, + pull_request: { + number: 60, + user: { login: 'legit' }, + state: 'open', + head: { repo: { full_name: 'QwenLM/qwen-code' } }, + }, }); assert.deepEqual(names(calls), ['pulls.deleteReviewComment']); }); @@ -601,6 +608,7 @@ describe('spam-blocklist-enforce: enforce lane behaviour', () => { number: 60, user: { login: 'spamuser' }, state: 'open', + head: { repo: { full_name: 'QwenLM/qwen-code' } }, }, }); assert.deepEqual(names(calls), ['pulls.update', 'issues.lock']); @@ -618,6 +626,7 @@ describe('spam-blocklist-enforce: enforce lane behaviour', () => { // 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), [ @@ -639,6 +648,7 @@ describe('spam-blocklist-enforce: enforce lane behaviour', () => { user: { login: 'spamuser' }, state: 'closed', locked: true, + head: { repo: { full_name: 'QwenLM/qwen-code' } }, }, }); assert.deepEqual(names(calls), ['pulls.deleteReviewComment']); @@ -653,6 +663,7 @@ describe('spam-blocklist-enforce: enforce lane behaviour', () => { user: { login: 'spamuser' }, state: 'open', locked: true, + head: { repo: { full_name: 'QwenLM/qwen-code' } }, }, }); assert.deepEqual(names(calls), [ @@ -695,6 +706,24 @@ describe('spam-blocklist-enforce: enforce lane behaviour', () => { 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('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' } }, @@ -720,6 +749,7 @@ describe('spam-blocklist-enforce: enforce lane behaviour', () => { number: 61, user: { login: 'spamuser' }, state: 'open', + head: { repo: { full_name: 'QwenLM/qwen-code' } }, }, }); assert.deepEqual(names(calls), ['pulls.update', 'issues.lock']); @@ -735,6 +765,7 @@ describe('spam-blocklist-enforce: enforce lane behaviour', () => { // 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), [ @@ -751,7 +782,12 @@ describe('spam-blocklist-enforce: enforce lane behaviour', () => { // 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' }, + pull_request: { + number: 61, + user: { login: 'legit' }, + state: 'open', + head: { repo: { full_name: 'QwenLM/qwen-code' } }, + }, }); assert.deepEqual(names(calls), ['graphql.minimizeComment']); }); @@ -767,6 +803,7 @@ describe('spam-blocklist-enforce: enforce lane behaviour', () => { user: { login: 'spamuser' }, state: 'closed', locked: true, + head: { repo: { full_name: 'QwenLM/qwen-code' } }, }, }); assert.deepEqual(names(calls), ['graphql.minimizeComment']); @@ -781,6 +818,7 @@ describe('spam-blocklist-enforce: enforce lane behaviour', () => { user: { login: 'spamuser' }, state: 'open', locked: true, + head: { repo: { full_name: 'QwenLM/qwen-code' } }, }, }); assert.deepEqual(names(calls), ['pulls.update', 'issues.lock']); @@ -803,6 +841,24 @@ describe('spam-blocklist-enforce: enforce lane behaviour', () => { 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('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 @@ -847,10 +903,12 @@ describe('spam-blocklist-enforce: enforce lane behaviour', () => { assert.ok(core.logs.info.some((m) => /already gone/.test(m))); }); - it('treats a 422 on issues.lock as already-done', async () => { + 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, and the desired end state is reached. - const { core } = await enforce( + // 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' } }, @@ -859,10 +917,153 @@ describe('spam-blocklist-enforce: enforce lane behaviour', () => { { fail: (name) => name === 'issues.lock' ? new HttpError(422, 'Already locked') : null, + replies: { 'issues.get': { locked: true } }, }, ); assert.deepEqual(core.logs.failed, []); - assert.ok(core.logs.info.some((m) => /already gone/.test(m))); + 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 () => { @@ -1042,7 +1243,12 @@ describe('spam-blocklist-enforce: enforce lane behaviour', () => { // 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' }, + 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); @@ -1056,6 +1262,7 @@ describe('spam-blocklist-enforce: sweep lane behaviour', () => { reviewComments = [], threads = [], fail, + replies, blocklist = BLOCKLIST, lookbackHours, } = {}) => { @@ -1068,6 +1275,7 @@ describe('spam-blocklist-enforce: sweep lane behaviour', () => { github: makeGithub({ calls, fail, + replies, pages: { 'issues.listCommentsForRepo': issueComments, 'pulls.listReviewCommentsForRepo': reviewComments, @@ -1245,16 +1453,45 @@ describe('spam-blocklist-enforce: sweep lane behaviour', () => { assert.ok(core.logs.info.some((m) => /already gone/.test(m))); }); - it('treats a sweep 422 on lock as already-done', async () => { + 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. - const { core } = await sweep({ + // 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, []); - assert.ok(core.logs.info.some((m) => /already gone/.test(m))); + 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('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 () => { @@ -1305,6 +1542,33 @@ describe('spam-blocklist-enforce: sweep lane behaviour', () => { }); } + // 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 }], diff --git a/.github/workflows/spam-blocklist-enforce.yml b/.github/workflows/spam-blocklist-enforce.yml index 64d7a366eb1..a2dc811ed0f 100644 --- a/.github/workflows/spam-blocklist-enforce.yml +++ b/.github/workflows/spam-blocklist-enforce.yml @@ -152,17 +152,19 @@ jobs: const actions = []; const failures = []; - const run = async (label, fn) => { + // 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 { await fn(); actions.push(label); core.info(`ok: ${label}`); } catch (error) { - // 404 means someone (a maintainer, the spammer, a concurrent - // run of this workflow) already removed it; 422 is what - // issues.lock returns when a concurrent run already locked - // the thread. Both are the desired end state, not a failure. - if (error?.status === 404 || error?.status === 422) { + if (tolerated.includes(error?.status)) { core.info(`already gone: ${label}`); return; } @@ -171,6 +173,23 @@ jobs: } }; + // 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. @@ -198,14 +217,22 @@ jobs: }), ); } - await run(`lock ${kind} #${number}`, () => - github.rest.issues.lock({ - owner, - repo, - issue_number: number, - lock_reason: 'spam', - }), - ); + 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') { @@ -240,10 +267,13 @@ jobs: // 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. + // 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?.head?.repo?.full_name && - pull.head.repo.full_name !== `${owner}/${repo}` && + pull && + (!pull.head?.repo || + pull.head.repo.full_name !== `${owner}/${repo}`) && (isBlocked(comment?.user?.login) || isBlocked(pull.user?.login)) ) { @@ -277,9 +307,12 @@ jobs: // 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?.head?.repo?.full_name && - pull.head.repo.full_name !== `${owner}/${repo}` && + pull && + (!pull.head?.repo || + pull.head.repo.full_name !== `${owner}/${repo}`) && (isBlocked(review?.user?.login) || isBlocked(pull.user?.login)) ) { @@ -290,7 +323,10 @@ jobs: } if (isBlocked(review?.user?.login)) { // No REST delete for a review body; SPAM-classify it instead. - await run(`minimize review ${review.id}`, () => + // 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}) { @@ -298,8 +334,8 @@ jobs: } }`, { id: review.node_id }, - ), - ); + ); + await run(`minimize review ${review.id}`, minimize, []); } if ( pull && @@ -316,11 +352,11 @@ jobs: } // Both empty means either no blocklisted author was involved or - // every needed action was already done (a 404/422 tolerated - // 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. + // 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; @@ -392,16 +428,20 @@ jobs: const actions = []; const failures = []; - const run = async (label, fn) => { + // 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) { - // 404 = already removed; 422 = issues.lock on a thread a - // concurrent run already locked. Desired end state both ways. - if (error?.status === 404 || error?.status === 422) { + if (tolerated.includes(error?.status)) { core.info(`already gone: ${label}`); return undefined; } @@ -411,6 +451,23 @@ jobs: } }; + // 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 @@ -419,15 +476,21 @@ jobs: // 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. + // 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, - }), + (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; @@ -441,13 +504,16 @@ jobs: } const reviewComments = - (await run('list review comments', () => - github.paginate(github.rest.pulls.listReviewCommentsForRepo, { - owner, - repo, - since, - per_page: 100, - }), + (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; @@ -470,16 +536,19 @@ jobs: // 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, - }), + (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; @@ -504,14 +573,24 @@ jobs: ); } if (!thread.locked) { - await run(`lock ${kind} #${thread.number}`, () => - github.rest.issues.lock({ - owner, - repo, - issue_number: thread.number, - lock_reason: 'spam', - }), - ); + 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; + } + }); } } From f59f1d9515accb73aaf93d9bcbd9faa3f60489bd Mon Sep 17 00:00:00 2001 From: qwen-code-dev-bot Date: Thu, 13 Aug 2026 23:51:14 +0000 Subject: [PATCH 09/10] fix(ci): apply round-8 spam blocklist review feedback (#8767) Co-authored-by: Qwen-Coder --- .../scripts/spam-blocklist-enforce.test.mjs | 73 ++++++++++++++++++- .github/workflows/spam-blocklist-enforce.yml | 28 ++++++- 2 files changed, 96 insertions(+), 5 deletions(-) diff --git a/.github/scripts/spam-blocklist-enforce.test.mjs b/.github/scripts/spam-blocklist-enforce.test.mjs index 8e6c33bcf88..599f748c6d8 100644 --- a/.github/scripts/spam-blocklist-enforce.test.mjs +++ b/.github/scripts/spam-blocklist-enforce.test.mjs @@ -523,6 +523,66 @@ describe('spam-blocklist-enforce: enforce lane behaviour', () => { assert.equal(calls[2].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' }, + }, + pull_request: { + number: 77, + 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, 77); + assert.equal(calls[1].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. + const { calls, core } = await enforce('issue_comment', { + comment: { id: 111, user: { login: 'spamuser' } }, + issue: { number: 5, user: { login: 'legit' }, state: 'open' }, + pull_request: { + number: 5, + user: { login: 'legit' }, + 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('skips a deleted-fork PR issue comment: head.repo is null', async () => { + // The deleted-fork twin: head.repo: null 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: { + number: 5, + 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('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. @@ -1600,9 +1660,10 @@ describe('spam-blocklist-enforce: sweep lane behaviour', () => { }); describe('spam-blocklist-enforce: blocklist file', () => { - it('embeds the same parser and isBlocked helper in both lanes', () => { + 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. + // 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} =`); @@ -1626,6 +1687,14 @@ describe('spam-blocklist-enforce: blocklist file', () => { 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', () => { diff --git a/.github/workflows/spam-blocklist-enforce.yml b/.github/workflows/spam-blocklist-enforce.yml index a2dc811ed0f..dfc3930c375 100644 --- a/.github/workflows/spam-blocklist-enforce.yml +++ b/.github/workflows/spam-blocklist-enforce.yml @@ -160,16 +160,18 @@ jobs: // back locked. const run = async (label, fn, tolerated = [404]) => { try { - await fn(); + 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; + return undefined; } failures.push(`${label}: ${error?.status ?? ''} ${error?.message ?? error}`); core.warning(`failed: ${label} — ${error?.message ?? error}`); + return undefined; } }; @@ -237,6 +239,27 @@ jobs: 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. A deleted fork arrives as head.repo: null with the + // same read-only downgrade, so a missing head repo counts as + // a fork. + if ( + payload.pull_request && + (!payload.pull_request.head?.repo || + payload.pull_request.head.repo.full_name !== `${owner}/${repo}`) && + (isBlocked(comment?.user?.login) || + isBlocked(issue?.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 issue comment ${comment.id}`, () => github.rest.issues.deleteComment({ @@ -252,7 +275,6 @@ jobs: // 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. - const issue = payload.issue; if ( issue && !(issue.state === 'closed' && issue.locked) && From f04d02fa06cfea3f9bd0231855670e9e89d88a0b Mon Sep 17 00:00:00 2001 From: qwen-code-dev-bot Date: Fri, 14 Aug 2026 04:06:41 +0000 Subject: [PATCH 10/10] fix(ci): apply round-9 spam blocklist review feedback (#8767) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Resolve the Critical finding: the issue_comment lane's fork-deferral guard read payload.pull_request.head?.repo, but issue_comment payloads carry no top-level pull_request object — the guard was dead code on the exact events it was written for, so every blocklisted interaction on a fork PR 403-red-ran the lane until the hourly sweep. Resolve PR-ness via issue.pull_request and the head repo via pulls.get, gated on blocklist involvement; deleted forks still read back as head.repo: null. Re-pin the two fork-skip tests against the real payload shape (they had fabricated the top-level pull_request the event never delivers) and add pulls.get to the fake Octokit. Address the actionable suggestions from the same review: - Exercise each fork-skip guard's thread-author disjunct alone in all three lanes (a mutation deleting it previously shipped green). - Pin the documented 404 tolerance of the close and lock calls in both lanes; until now only delete-call 404s were pinned. - Add a static assertion that no ${{ }} expression appears inside a github-script body — the one position where the runner interpolates event-controlled text into code parsed on the write token. - Model fork heads on the pull_request_target behavioural fixtures so a head-presence deferral guard cannot silently no-op the lane that exists precisely so fork PRs are closable. - Let fake Octokit replies be functions of the call params and pin per-thread lock-422 readback verdicts in a two-thread sweep, so one thread's verdict cannot leak onto the other. --- .../scripts/spam-blocklist-enforce.test.mjs | 371 +++++++++++++++--- .github/workflows/spam-blocklist-enforce.yml | 30 +- 2 files changed, 329 insertions(+), 72 deletions(-) diff --git a/.github/scripts/spam-blocklist-enforce.test.mjs b/.github/scripts/spam-blocklist-enforce.test.mjs index 599f748c6d8..70416787d90 100644 --- a/.github/scripts/spam-blocklist-enforce.test.mjs +++ b/.github/scripts/spam-blocklist-enforce.test.mjs @@ -222,6 +222,21 @@ describe('spam-blocklist-enforce: script wiring', () => { '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', () => { @@ -351,13 +366,18 @@ const makeCore = () => { // 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 lock-race read-back (issues.get). +// 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; - return { data: replies[name] ?? {} }; + const reply = replies[name]; + return { + data: typeof reply === 'function' ? reply(params) : (reply ?? {}), + }; }; return { rest: { @@ -374,6 +394,7 @@ const makeGithub = ({ calls, fail = () => null, pages = {}, replies = {} }) => { pulls: { deleteReviewComment: record('pulls.deleteReviewComment'), update: record('pulls.update'), + get: record('pulls.get'), listReviewCommentsForRepo: 'pulls.listReviewCommentsForRepo', }, }, @@ -460,16 +481,24 @@ describe('spam-blocklist-enforce: enforce lane behaviour', () => { 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: {}, + const { calls } = await enforce( + 'issue_comment', + { + comment: { id: 9, user: { login: 'spamuser' } }, + issue: { + number: 8626, + user: { login: 'legit' }, + state: 'open', + pull_request: {}, + }, }, - }); - assert.deepEqual(names(calls), ['issues.deleteComment']); + { + 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 () => { @@ -504,81 +533,137 @@ describe('spam-blocklist-enforce: enforce lane behaviour', () => { }); 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' }, + 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[1].params.pull_number, 77); - assert.equal(calls[1].params.state, 'closed'); - assert.equal(calls[2].params.issue_number, 77); + 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' }, + const { calls } = await enforce( + 'issue_comment', + { + comment: { id: 9, user: { login: 'legit' } }, + issue: { + number: 77, + user: { login: 'spamuser' }, + state: 'open', + pull_request: { url: 'x' }, + }, }, - pull_request: { - number: 77, - user: { login: 'spamuser' }, - state: 'open', - head: { repo: { full_name: 'QwenLM/qwen-code' } }, + { + replies: { + 'pulls.get': { head: { repo: { full_name: 'QwenLM/qwen-code' } } }, + }, }, - }); - assert.deepEqual(names(calls), ['pulls.update', 'issues.lock']); - assert.equal(calls[0].params.pull_number, 77); - assert.equal(calls[1].params.issue_number, 77); + ); + 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. - const { calls, core } = await enforce('issue_comment', { - comment: { id: 111, user: { login: 'spamuser' } }, - issue: { number: 5, user: { login: 'legit' }, state: 'open' }, - pull_request: { - number: 5, - user: { login: 'legit' }, - state: 'open', - head: { repo: { full_name: 'forker/qwen-code' } }, + // 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' }, + }, }, - }); - assert.deepEqual(names(calls), []); + { + 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: head.repo: null 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: { - number: 5, - user: { login: 'spamuser' }, - state: 'open', - head: { repo: null }, + // 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' }, + }, }, - }); - assert.deepEqual(names(calls), []); + { 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))); }); @@ -784,6 +869,24 @@ describe('spam-blocklist-enforce: enforce lane behaviour', () => { 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' } }, @@ -919,6 +1022,25 @@ describe('spam-blocklist-enforce: enforce lane behaviour', () => { 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 @@ -926,6 +1048,7 @@ describe('spam-blocklist-enforce: enforce lane behaviour', () => { // 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), []); @@ -935,7 +1058,13 @@ describe('spam-blocklist-enforce: enforce lane behaviour', () => { 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. - pull_request: { number: 101, user: { login: 'SpAmUsEr' } }, + // 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); @@ -943,6 +1072,21 @@ describe('spam-blocklist-enforce: enforce lane behaviour', () => { 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' }, @@ -1150,6 +1294,56 @@ describe('spam-blocklist-enforce: enforce lane behaviour', () => { ]); }); + 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 @@ -1513,6 +1707,29 @@ describe('spam-blocklist-enforce: sweep lane behaviour', () => { 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 @@ -1539,6 +1756,36 @@ describe('spam-blocklist-enforce: sweep lane behaviour', () => { 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 diff --git a/.github/workflows/spam-blocklist-enforce.yml b/.github/workflows/spam-blocklist-enforce.yml index dfc3930c375..bfddcd380db 100644 --- a/.github/workflows/spam-blocklist-enforce.yml +++ b/.github/workflows/spam-blocklist-enforce.yml @@ -245,20 +245,30 @@ jobs: // 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. A deleted fork arrives as head.repo: null with the - // same read-only downgrade, so a missing head repo counts as - // a fork. + // 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 ( - payload.pull_request && - (!payload.pull_request.head?.repo || - payload.pull_request.head.repo.full_name !== `${owner}/${repo}`) && + issue?.pull_request && (isBlocked(comment?.user?.login) || isBlocked(issue?.user?.login)) ) { - core.notice( - 'fork PR: GITHUB_TOKEN is read-only here; deferring to the sweep lane', - ); - return; + 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}`, () =>