From ecebd2d23e1c55873a45cab68f3ffdc7501e263a Mon Sep 17 00:00:00 2001 From: Cindy Zhang Date: Mon, 31 Aug 2026 21:48:30 -0700 Subject: [PATCH] fix(ci): avoid stale review signal cancellation --- .github/scripts/review-signal-policy.cjs | 60 +++++ .github/scripts/review-signal-policy.test.mjs | 190 ++++++++++++++++ .github/workflows/review-signal.yml | 207 +++++++++++++----- 3 files changed, 399 insertions(+), 58 deletions(-) create mode 100644 .github/scripts/review-signal-policy.cjs create mode 100644 .github/scripts/review-signal-policy.test.mjs diff --git a/.github/scripts/review-signal-policy.cjs b/.github/scripts/review-signal-policy.cjs new file mode 100644 index 0000000000000..22654e8c11ffb --- /dev/null +++ b/.github/scripts/review-signal-policy.cjs @@ -0,0 +1,60 @@ +// Copyright (c) Meta Platforms, Inc. and affiliates. + +/** + * @input A review-signal event identity, the current PR head, and a proposed mutation. + * @output A fail-closed event plan and a current-head-guarded mutation result. + * @position Trusted policy shared by review-signal.yml and its workflow contract tests. + * + * SYNC: review-signal.yml loads this file with new Function(module, exports, + * require) — keep it dependency-free CJS with a plain module.exports object. + */ + +const EFFECT_BEARING_PULL_REQUEST_ACTIONS = new Set([ + 'opened', + 'synchronize', + 'reopened', +]); + +function reviewSignalEventPlan({ + eventName, + eventAction, + eventHeadSha, + currentHeadSha, +}) { + if (eventName === 'workflow_dispatch') { + return {shouldMutate: true, reason: 'manual-dispatch'}; + } + if (eventName !== 'pull_request_target') { + return {shouldMutate: false, reason: 'non-mutating-event'}; + } + if (!EFFECT_BEARING_PULL_REQUEST_ACTIONS.has(eventAction)) { + return {shouldMutate: false, reason: 'no-op-action'}; + } + if (!eventHeadSha || !currentHeadSha || eventHeadSha !== currentHeadSha) { + return {shouldMutate: false, reason: 'superseded-head'}; + } + return {shouldMutate: true, reason: 'current-head'}; +} + +async function mutateIfCurrentHead({expectedHead, getCurrentHead, mutate}) { + if ( + !expectedHead || + typeof getCurrentHead !== 'function' || + typeof mutate !== 'function' + ) { + throw new TypeError( + 'mutateIfCurrentHead requires an expected head and read/mutation functions.', + ); + } + const currentHead = await getCurrentHead(); + if (currentHead !== expectedHead) { + return {applied: false, currentHead}; + } + await mutate(); + return {applied: true, currentHead}; +} + +module.exports = { + mutateIfCurrentHead, + reviewSignalEventPlan, +}; diff --git a/.github/scripts/review-signal-policy.test.mjs b/.github/scripts/review-signal-policy.test.mjs new file mode 100644 index 0000000000000..5674e9fffdd74 --- /dev/null +++ b/.github/scripts/review-signal-policy.test.mjs @@ -0,0 +1,190 @@ +// Copyright (c) Meta Platforms, Inc. and affiliates. + +/** + * @input The review-signal event policy and trusted workflow source. + * @output Regression coverage for duplicate events, superseded heads, and guarded writes. + * @position Mutation-sensitive contract tests for review-signal.yml. + */ + +import fs from 'node:fs'; +import path from 'node:path'; +import {createRequire} from 'node:module'; +import {fileURLToPath} from 'node:url'; + +import {describe, expect, it} from 'vitest'; + +const require = createRequire(import.meta.url); +const { + mutateIfCurrentHead, + reviewSignalEventPlan, +} = require('./review-signal-policy.cjs'); + +const HEAD = 'a'.repeat(40); +const NEW_HEAD = 'b'.repeat(40); +const WORKFLOW = path.resolve( + path.dirname(fileURLToPath(import.meta.url)), + '../workflows/review-signal.yml', +); + +function pullRequestPlan( + eventAction, + eventHeadSha = HEAD, + currentHeadSha = HEAD, +) { + return reviewSignalEventPlan({ + eventName: 'pull_request_target', + eventAction, + eventHeadSha, + currentHeadSha, + }); +} + +describe('review signal event policy', () => { + it('runs only the effect-bearing event in the synchronize plus ready duplicate', () => { + expect(pullRequestPlan('synchronize')).toEqual({ + shouldMutate: true, + reason: 'current-head', + }); + expect(pullRequestPlan('ready_for_review')).toEqual({ + shouldMutate: false, + reason: 'no-op-action', + }); + }); + + it('skips an effect-bearing event after its head is superseded', () => { + expect(pullRequestPlan('synchronize', HEAD, NEW_HEAD)).toEqual({ + shouldMutate: false, + reason: 'superseded-head', + }); + }); + + it('keeps current head updates and explicit recovery effect-bearing', () => { + expect(pullRequestPlan('opened').shouldMutate).toBe(true); + expect(pullRequestPlan('reopened').shouldMutate).toBe(true); + expect( + reviewSignalEventPlan({eventName: 'workflow_dispatch'}).shouldMutate, + ).toBe(true); + }); +}); + +describe('review signal mutation guard', () => { + it('applies an effect for the current head', async () => { + const writes = []; + const result = await mutateIfCurrentHead({ + expectedHead: HEAD, + getCurrentHead: async () => HEAD, + mutate: async () => writes.push('status'), + }); + + expect(result).toEqual({applied: true, currentHead: HEAD}); + expect(writes).toEqual(['status']); + }); + + it('does not write after the planned head has been superseded', async () => { + const plan = pullRequestPlan('synchronize'); + expect(plan.shouldMutate).toBe(true); + const writes = []; + + const result = await mutateIfCurrentHead({ + expectedHead: HEAD, + getCurrentHead: async () => NEW_HEAD, + mutate: async () => writes.push('stale-status'), + }); + + expect(result).toEqual({applied: false, currentHead: NEW_HEAD}); + expect(writes).toEqual([]); + }); + + it('rechecks the head before every separate mutation', async () => { + let currentHead = HEAD; + const writes = []; + const first = await mutateIfCurrentHead({ + expectedHead: HEAD, + getCurrentHead: async () => currentHead, + mutate: async () => writes.push('label'), + }); + currentHead = NEW_HEAD; + const second = await mutateIfCurrentHead({ + expectedHead: HEAD, + getCurrentHead: async () => currentHead, + mutate: async () => writes.push('status'), + }); + + expect(first.applied).toBe(true); + expect(second.applied).toBe(false); + expect(writes).toEqual(['label']); + }); +}); + +describe('review signal workflow contract', () => { + it('loads the policy through the dependency-free trusted-base boundary', () => { + const source = fs.readFileSync( + fileURLToPath(new URL('./review-signal-policy.cjs', import.meta.url)), + 'utf8', + ); + const mod = {exports: {}}; + new Function('module', 'exports', 'require', source)( + mod, + mod.exports, + () => { + throw new Error('review-signal-policy.cjs must stay dependency-free'); + }, + ); + + expect(mod.exports.reviewSignalEventPlan).toBeTypeOf('function'); + expect(mod.exports.mutateIfCurrentHead).toBeTypeOf('function'); + }); + + it('does not start lifecycle-only duplicates or let review anchors cancel flagging', () => { + const workflow = fs.readFileSync(WORKFLOW, 'utf8'); + const [header, jobs] = workflow.split('\njobs:\n'); + const flag = jobs.slice(0, jobs.indexOf('\n review-anchor:')); + const anchor = jobs.slice(jobs.indexOf('\n review-anchor:')); + + expect(header).toContain('types: [opened, synchronize, reopened]'); + expect(header).not.toContain('ready_for_review'); + expect(header).not.toContain('converted_to_draft'); + expect(header).not.toContain('\nconcurrency:'); + expect(flag).toContain('group: review-signal-'); + expect(flag).toContain('cancel-in-progress: false'); + expect(anchor).not.toContain('concurrency:'); + }); + + it('plans from the event head and guards every mutation against the live head', () => { + const workflow = fs.readFileSync(WORKFLOW, 'utf8'); + const flag = workflow.slice( + workflow.indexOf(' flag:'), + workflow.indexOf(' review-anchor:'), + ); + + expect(flag).toContain("path: '.github/scripts/review-signal-policy.cjs'"); + expect(flag).toContain('reviewSignalEventPlan({'); + expect(flag).toContain('eventHeadSha: eventPr.head.sha'); + expect(flag).toContain('currentHeadSha: currentPr.head.sha'); + expect(flag).toContain('mutateIfCurrentHead({'); + expect(flag).not.toContain('actions/checkout'); + + const mutationMethods = [ + 'github.rest.issues.addLabels', + 'github.rest.issues.removeLabel', + 'github.rest.pulls.requestReviewers', + 'github.rest.repos.createCommitStatus', + 'github.rest.checks.update', + 'github.graphql', + ]; + for (const method of mutationMethods) { + const indexes = []; + let index = flag.indexOf(method); + while (index !== -1) { + indexes.push(index); + index = flag.indexOf(method, index + method.length); + } + expect(indexes.length).toBeGreaterThan(0); + for (const mutationIndex of indexes) { + expect( + flag.slice(Math.max(0, mutationIndex - 240), mutationIndex), + ).toContain('applyForCurrentHead('); + } + } + }); +}); diff --git a/.github/workflows/review-signal.yml b/.github/workflows/review-signal.yml index 3950a3fa7a285..ba3ed40e76458 100644 --- a/.github/workflows/review-signal.yml +++ b/.github/workflows/review-signal.yml @@ -59,7 +59,10 @@ name: Review signal on: pull_request_target: branches: ['main'] - types: [opened, synchronize, reopened, ready_for_review, converted_to_draft] + # Draft/ready transitions do not change review classification. Excluding + # them avoids a same-head lifecycle event cancelling the synchronize run + # that carries the actual update. + types: [opened, synchronize, reopened] pull_request_review: types: [submitted] # Manual re-trigger. The App only auto-runs on the PR events above, so if it @@ -76,12 +79,6 @@ on: permissions: {} -concurrency: - # One group per PR for the event/single-dispatch paths; a run-scoped group for - # the backfill-all dispatch so it is never cancelled by an unrelated re-run. - group: review-signal-${{ github.event.pull_request.number || github.event.inputs.pr || github.run_id }} - cancel-in-progress: true - env: CODE_LABEL: 'needs:code-review' DESIGN_LABEL: 'needs:design-review' @@ -96,6 +93,15 @@ jobs: github.event_name == 'pull_request_target' || github.event_name == 'workflow_dispatch' name: Flag review signals + # Serialize effect-bearing flag jobs instead of cancelling them. Every write + # rechecks the live head, so a superseded run exits successfully without + # overwriting the newer head's result. The read-only review anchor does not + # join this group. + concurrency: + # One group per PR for event/single-dispatch paths; a run-scoped group for + # backfill-all so an unrelated re-run cannot delay it. + group: review-signal-${{ github.event.pull_request.number || github.event.inputs.pr || github.run_id }} + cancel-in-progress: false runs-on: ubuntu-slim permissions: pull-requests: write @@ -119,9 +125,33 @@ jobs: const DESIGN = process.env.DESIGN_LABEL; const COMMUNITY = process.env.COMMUNITY_LABEL; - // Resolve which PR(s) to flag. On a PR event we get the payload PR. - // On manual dispatch we take the `pr` input (one PR), or re-flag - // every open PR when it's blank (backfill / mass recovery). + // Load the event and mutation policy from the trusted base branch. + // This workflow never checks out or executes PR-controlled code. + const policyRef = context.payload.pull_request?.base?.ref || + context.payload.repository.default_branch; + const { data: policyFile } = await github.rest.repos.getContent({ + owner, + repo, + ref: policyRef, + path: '.github/scripts/review-signal-policy.cjs', + }); + if (Array.isArray(policyFile) || !policyFile.content) { + throw new Error('Trusted review-signal policy is unavailable.'); + } + const policySource = Buffer.from(policyFile.content, 'base64').toString('utf8'); + const policyModule = { exports: {} }; + new Function('module', 'exports', 'require', policySource)( + policyModule, + policyModule.exports, + () => { throw new Error('review-signal-policy.cjs must stay dependency-free'); }, + ); + const { mutateIfCurrentHead, reviewSignalEventPlan } = policyModule.exports; + + // Resolve which PR(s) to flag. On a PR event, compare the event head + // with the live PR before classification. Lifecycle-only or already + // superseded events finish successfully without any mutation. + // On manual dispatch, take the `pr` input (one PR), or re-flag every + // open PR when it is blank (backfill / mass recovery). let prs; if (context.eventName === 'workflow_dispatch') { const num = (context.payload.inputs.pr || '').trim(); @@ -137,7 +167,21 @@ jobs: core.info(`Backfill: re-flagging ${prs.length} open PR(s).`); } } else { - prs = [context.payload.pull_request]; + const eventPr = context.payload.pull_request; + const { data: currentPr } = await github.rest.pulls.get({ + owner, + repo, + pull_number: eventPr.number, + }); + const plan = reviewSignalEventPlan({ + eventName: context.eventName, + eventAction: context.payload.action, + eventHeadSha: eventPr.head.sha, + currentHeadSha: currentPr.head.sha, + }); + core.info(`Event plan: ${plan.reason}.`); + if (!plan.shouldMutate) return; + prs = [currentPr]; } let hadError = false; @@ -153,6 +197,31 @@ jobs: // ================= per-PR detection + routing ================= async function flagOne(pr) { + const expectedHead = pr.head.sha; + let superseded = false; + async function applyForCurrentHead(description, mutate) { + if (superseded) return false; + const result = await mutateIfCurrentHead({ + expectedHead, + getCurrentHead: async () => { + const { data: current } = await github.rest.pulls.get({ + owner, + repo, + pull_number: pr.number, + }); + return current.head.sha; + }, + mutate, + }); + if (!result.applied) { + superseded = true; + core.info( + `Skipping ${description}: ${expectedHead} was superseded by ${result.currentHead}.`, + ); + } + return result.applied; + } + const author = pr.user.login.toLowerCase(); // Bots (Dependabot, github-actions, etc.) are our own automation, // not community contributors, and their PRs (dep bumps, chores) are @@ -197,19 +266,23 @@ jobs: for (const name of [CODE, DESIGN, COMMUNITY]) { if (current.includes(name)) { try { - await github.rest.issues.removeLabel({ - owner, repo, issue_number: pr.number, name, - }); + await applyForCurrentHead(`remove ${name}`, () => + github.rest.issues.removeLabel({ + owner, repo, issue_number: pr.number, name, + }), + ); } catch (e) { core.warning(`Could not remove ${name}: ${e.message}`); } } } - await github.rest.repos.createCommitStatus({ - owner, repo, sha: pr.head.sha, - context: 'review-required', state: 'success', - description: 'Automated (bot) PR — exempt from review gates.', - }); + await applyForCurrentHead('mark the bot review gate successful', () => + github.rest.repos.createCommitStatus({ + owner, repo, sha: expectedHead, + context: 'review-required', state: 'success', + description: 'Automated (bot) PR — exempt from review gates.', + }), + ); return; } @@ -424,17 +497,21 @@ jobs: // (authorIsOwner is defined above.) if (!authorIsOwner && !current.includes(COMMUNITY)) toAdd.push(COMMUNITY); if (toAdd.length) { - await github.rest.issues.addLabels({ - owner, repo, issue_number: pr.number, labels: toAdd, - }); + await applyForCurrentHead(`add ${toAdd.join(', ')}`, () => + github.rest.issues.addLabels({ + owner, repo, issue_number: pr.number, labels: toAdd, + }), + ); } // Keep it accurate: if an owner's PR somehow carries it (e.g. author // was added to an owners file after opening), drop it. if (authorIsOwner && current.includes(COMMUNITY)) { try { - await github.rest.issues.removeLabel({ - owner, repo, issue_number: pr.number, name: COMMUNITY, - }); + await applyForCurrentHead(`remove ${COMMUNITY}`, () => + github.rest.issues.removeLabel({ + owner, repo, issue_number: pr.number, name: COMMUNITY, + }), + ); } catch (e) { core.warning(`Could not remove ${COMMUNITY}: ${e.message}`); } @@ -444,10 +521,12 @@ jobs: // fire for forks, so we reconcile here from the reviews we read). async function dropLabel(name) { try { - await github.rest.issues.removeLabel({ - owner, repo, issue_number: pr.number, name, - }); - core.info(`Cleared ${name} (approved).`); + const applied = await applyForCurrentHead(`remove ${name}`, () => + github.rest.issues.removeLabel({ + owner, repo, issue_number: pr.number, name, + }), + ); + if (applied) core.info(`Cleared ${name} (approved).`); } catch (e) { core.warning(`Could not remove ${name}: ${e.message}`); } @@ -464,9 +543,11 @@ jobs: const reviewers = [...new Set(handles)].filter((h) => h !== author); if (!reviewers.length) return; try { - await github.rest.pulls.requestReviewers({ - owner, repo, pull_number: pr.number, reviewers, - }); + await applyForCurrentHead('request design reviewers', () => + github.rest.pulls.requestReviewers({ + owner, repo, pull_number: pr.number, reviewers, + }), + ); } catch (e) { core.warning(`requestReviewers(${reviewers.join(',')}) failed: ${e.message}`); } @@ -478,15 +559,17 @@ jobs: // disable auto-merge or block the merge. --- if (codeGated && pr.auto_merge) { try { - await github.graphql( - `mutation ($id: ID!) { - disablePullRequestAutoMerge(input: { pullRequestId: $id }) { - pullRequest { number } - } - }`, - { id: pr.node_id }, + await applyForCurrentHead('disable auto-merge', () => + github.graphql( + `mutation ($id: ID!) { + disablePullRequestAutoMerge(input: { pullRequestId: $id }) { + pullRequest { number } + } + }`, + { id: pr.node_id }, + ), ); - core.info('Disabled auto-merge (code review required).'); + if (!superseded) core.info('Disabled auto-merge (code review required).'); } catch (e) { core.warning(`Could not disable auto-merge: ${e.message}`); } @@ -513,33 +596,41 @@ jobs: // whose POST failed is blocked on a status that will never arrive. // Throwing marks the run red and lets the per-PR catch continue a // backfill; re-run the workflow to retry. - await github.rest.repos.createCommitStatus({ - owner, - repo, - sha: pr.head.sha, - context: 'review-required', - state: codeGated ? 'pending' : 'success', - description: codeGated - ? `Waiting on code review: ${gateReasons.join(', ')}`.slice(0, 140) - : 'No code review required.', - }); - core.info(`review-required status: ${codeGated ? 'pending' : 'success'}`); + const statusApplied = await applyForCurrentHead('publish review-required status', () => + github.rest.repos.createCommitStatus({ + owner, + repo, + sha: expectedHead, + context: 'review-required', + state: codeGated ? 'pending' : 'success', + description: codeGated + ? `Waiting on code review: ${gateReasons.join(', ')}`.slice(0, 140) + : 'No code review required.', + }), + ); + if (statusApplied) { + core.info(`review-required status: ${codeGated ? 'pending' : 'success'}`); + } // Neutralize any stale "review-required" CHECK RUN left over from the // pre-status gate. Such a leftover (conclusion action_required) drags // the status rollup to FAILURE even though we now gate via a commit // status — so once the status is set, clear the old check-run. try { const { data } = await github.rest.checks.listForRef({ - owner, repo, ref: pr.head.sha, check_name: 'review-required', per_page: 100, + owner, repo, ref: expectedHead, check_name: 'review-required', per_page: 100, }); for (const cr of data.check_runs) { if (cr.conclusion && cr.conclusion !== 'success' && cr.conclusion !== 'neutral') { - await github.rest.checks.update({ - owner, repo, check_run_id: cr.id, - conclusion: 'neutral', - output: { title: 'Superseded by review-required status', summary: 'The gate is now a review-required commit status; see that context.' }, - }); - core.info(`Neutralized stale review-required check-run ${cr.id}.`); + await applyForCurrentHead(`neutralize stale check-run ${cr.id}`, () => + github.rest.checks.update({ + owner, repo, check_run_id: cr.id, + conclusion: 'neutral', + output: { title: 'Superseded by review-required status', summary: 'The gate is now a review-required commit status; see that context.' }, + }), + ); + if (!superseded) { + core.info(`Neutralized stale review-required check-run ${cr.id}.`); + } } } } catch (e) {