diff --git a/.github/WORKFLOWS.md b/.github/WORKFLOWS.md index f3a3f9d8a26..2b926ba06b9 100644 --- a/.github/WORKFLOWS.md +++ b/.github/WORKFLOWS.md @@ -889,14 +889,42 @@ PR changes and review events, and reports a commit status named **Required Reviews** on the head SHA. A missing approval reports `pending` ("Waiting for approval from: …"), not `failure`, so an unreviewed PR does not show red CI; any non-success state blocks the merge equally. The -ruleset for `master` must list -that status as a required check for the block to take effect. Merge-queue runs -report success on the queue head without re-evaluating: a PR cannot enter the -queue unless the status is green on its head, and the queue does not change -approvals. +ruleset for a branch must list that status as a required check for the block +to take effect. Merge-queue runs report success on the queue head without +re-evaluating: a PR cannot enter the queue unless the status is green on its +head, and the queue does not change approvals. -The workflow reads OWNERS and its scripts from the base branch only, so a PR -cannot lift its own review requirement. +The status is evaluated for a PR into any base branch, from a same-repo head +or a fork. Both matter because the ruleset that gates a PR is not always the +one on its base branch: GitHub applies the ruleset of a stack's target branch +to every PR in the stack, so a stacked PR into a feature branch is gated by +the `master` ruleset. A required status that no run ever writes leaves the PR +blocked on "Expected". Routes that skip the evaluation are listed in +`REQUIRED_REVIEW_EXEMPTIONS` in +`required-reviews.mjs`. An entry is ` -> ` or just `` +(any head); `*` matches any run of characters. An exempt PR reports +`success` with the route in the description. Only heads in this repository +can match, so a fork branch with a matching name is still evaluated. Add a +route only when every commit it carries was already reviewed elsewhere, as +with `sync/master-to-3x -> 3.x`: its commits landed on `master` first. + +The workflow reads OWNERS, its scripts and the exemption routes from `master` +only, never from the base branch or the PR: any writable branch can be a base, +so only `master` is trusted input. A PR cannot lift its own review +requirement. A retarget re-evaluates the PR, so a verdict computed against the +old base does not carry over. + +Every path that writes the status runs in the base repository context, because +a fork-context run has no secrets and a read-only token. PR changes arrive +through `pull_request_target`, which is safe here because no step checks out +or runs PR code. Review events on a same-repo PR arrive through +`pull_request_review`. Review events on a fork PR arrive through the +`workflow_run` of `ci-pull-request-review.yml`, which runs on every submitted +or dismissed review; the owners workflow looks the PR up from that run's head +and skips same-repo heads, which the direct event already covers. A first +contribution whose runs still wait for approval gets no review-event +re-evaluation until a maintainer approves the runs; `workflow_dispatch` with +the PR number is the manual fallback. ### Transition from CODEOWNERS diff --git a/.github/scripts/github-helpers.mjs b/.github/scripts/github-helpers.mjs index bb2430deece..c579de3423d 100644 --- a/.github/scripts/github-helpers.mjs +++ b/.github/scripts/github-helpers.mjs @@ -414,6 +414,26 @@ export async function getPullRequestById(pullRequestId) { return pullRequest.data; } +/** + * Open PRs whose head is `headOwner:headBranch`, newest first. + * + * @param { string } headOwner Owner of the head repository (a fork owner or this org). + * @param { string } headBranch + * @returns { Promise } + */ +export async function listOpenPullRequestsByHead(headOwner, headBranch) { + const { octokit, owner, repo } = initGithub(); + + const pullRequests = await octokit.rest.pulls.list({ + owner, + repo, + state: 'open', + head: `${headOwner}:${headBranch}`, + }); + + return pullRequests.data; +} + /** * Returns the set of files changed in a PR, including previous filenames for renames. * diff --git a/.github/scripts/owners/required-reviews.mjs b/.github/scripts/owners/required-reviews.mjs index e0e624b292f..863978df58b 100644 --- a/.github/scripts/owners/required-reviews.mjs +++ b/.github/scripts/owners/required-reviews.mjs @@ -8,6 +8,9 @@ * runs do not reach this script: the workflow reports success on the queue * head directly, because entering the queue already required a green status. * + * Every base branch is enforced unless the PR matches a route in + * REQUIRED_REVIEW_EXEMPTIONS; an exempt PR reports success without evaluation. + * * The job itself always succeeds when the evaluation runs; the commit status * carries the verdict. The status is set to pending before the evaluation * starts, so a crash (e.g. the team membership API is unavailable) cannot @@ -20,12 +23,30 @@ import { getPrReviews, getPullRequestById, isTeamMember, + listOpenPullRequestsByHead, setCommitStatus, } from '../github-helpers.mjs'; import { parseOwnersFile, resolveRequiredTeams, teamHandleToSlug } from './owners.mjs'; export const STATUS_CONTEXT = 'Required Reviews'; -const TARGET_BRANCH = 'master'; + +/** + * PR routes that skip the evaluation. Each entry is ` -> ` or + * just `` (any head). `*` matches any run of characters, including + * `/`. Only heads in this repository qualify; a fork branch with a + * matching name does not. + * + * Add a route here only when every commit it carries was already reviewed + * elsewhere, like the master-to-3.x sync, whose commits landed on master. + */ +export const REQUIRED_REVIEW_EXEMPTIONS = ['sync/master-to-3x -> 3.x']; + +/** + * @typedef ExemptRoute + * @property { string } head Glob for the head branch. + * @property { string } base Glob for the base branch. + * @property { string } source The entry as written. + */ /** * A PR review as returned by the reviews API. @@ -36,17 +57,68 @@ const TARGET_BRANCH = 'master'; * @property { string } [submitted_at] */ +/** + * @param { string } entry ` -> ` or `` + * @returns { ExemptRoute } + */ +export function parseExemption(entry) { + const parts = entry.split('->').map((part) => part.trim()); + if (parts.length > 2 || parts.some((part) => part === '')) { + throw new Error(`Invalid exemption "${entry}": expected " -> " or ""`); + } + const [head, base] = parts.length === 2 ? parts : ['*', parts[0]]; + return { head, base, source: entry }; +} + +/** + * @param { string } ref Branch name. + * @param { string } pattern Glob where `*` matches any run of characters. + * @returns { boolean } + */ +export function matchesBranchPattern(ref, pattern) { + const escaped = pattern.split('*').map((part) => part.replace(/[.*+?^${}()|[\]\\]/g, '\\$&')); + return new RegExp(`^${escaped.join('.*')}$`).test(ref); +} + +/** + * @param { { head: { ref: string, repo: { full_name: string } | null }, base: { ref: string, repo: { full_name: string } } } } pullRequest + * @param { string[] } [exemptions] + * @returns { ExemptRoute | undefined } The first matching route. + */ +export function findExemption(pullRequest, exemptions = REQUIRED_REVIEW_EXEMPTIONS) { + // A fork can name its branch anything, so the head must live in this repo. + if (pullRequest.head.repo?.full_name !== pullRequest.base.repo.full_name) return undefined; + + return exemptions + .map(parseExemption) + .find( + (route) => + matchesBranchPattern(pullRequest.head.ref, route.head) && + matchesBranchPattern(pullRequest.base.ref, route.base), + ); +} + /** * Resolve the PR number from the triggering event. * + * A workflow_run event carries no PR: the PR is looked up from the + * triggering run's head. Only an open PR whose head SHA is the run's head + * SHA counts; several PRs can share a head branch. + * * @param { string } eventName * @param { any } event Parsed GITHUB_EVENT_PATH payload. * @param { string | undefined } pullRequestNumberEnv PULL_REQUEST_NUMBER (workflow_dispatch input). - * @returns { number } + * @returns { Promise } undefined when the event has no open PR to evaluate. */ -export function resolvePullRequestNumber(eventName, event, pullRequestNumberEnv) { +export async function resolvePullRequestNumber(eventName, event, pullRequestNumberEnv) { if (event?.pull_request?.number) return event.pull_request.number; + if (eventName === 'workflow_run' && event?.workflow_run) { + const { head_sha: sha, head_branch: branch, head_repository: headRepository } = event.workflow_run; + const candidates = await listOpenPullRequestsByHead(headRepository.owner.login, branch); + return candidates.find((pullRequest) => pullRequest.head.sha === sha)?.number; + } + const parsed = parseInt(pullRequestNumberEnv ?? ''); if (Number.isNaN(parsed)) { throw new Error(`Cannot resolve a PR number for event "${eventName}"`); @@ -160,18 +232,16 @@ export async function run() { const eventName = process.env.GITHUB_EVENT_NAME ?? 'workflow_dispatch'; const event = getEventFromGithubEventPath(); - const pullRequestNumber = resolvePullRequestNumber( + const pullRequestNumber = await resolvePullRequestNumber( eventName, event, process.env.PULL_REQUEST_NUMBER, ); - const pullRequest = await getPullRequestById(pullRequestNumber); - - if (pullRequest.base.ref !== TARGET_BRANCH) { - console.log(`PR #${pullRequestNumber} targets "${pullRequest.base.ref}", not "${TARGET_BRANCH}"; skipping.`); + if (pullRequestNumber === undefined) { + console.log(`No open PR for the ${eventName} head; nothing to evaluate.`); return; } - + const pullRequest = await getPullRequestById(pullRequestNumber); const statusSha = pullRequest.head.sha; const targetUrl = statusTargetUrl(); @@ -187,7 +257,13 @@ export async function run() { /** @type { { state: 'success' | 'pending', description: string } } */ let status; try { - status = await evaluateRequiredReviews(pullRequestNumber); + const exemption = findExemption(pullRequest); + if (exemption) { + console.log(`PR #${pullRequestNumber} matches exempt route "${exemption.source}"; skipping the evaluation.`); + status = { state: 'success', description: `Exempt route: ${exemption.source}` }; + } else { + status = await evaluateRequiredReviews(pullRequestNumber); + } } catch (evaluationError) { // Best effort: nicer than a stuck pending status. The pending status // already blocks the merge if this write fails too. diff --git a/.github/scripts/owners/required-reviews.test.mjs b/.github/scripts/owners/required-reviews.test.mjs index 1b5bd7885ee..3b20341cb6c 100644 --- a/.github/scripts/owners/required-reviews.test.mjs +++ b/.github/scripts/owners/required-reviews.test.mjs @@ -19,6 +19,8 @@ let getPrReviewsImpl = async () => []; let isTeamMemberImpl = async () => false; /** @type {(sha: string, status: any) => Promise} */ let setCommitStatusImpl = async () => {}; +/** @type {(headOwner: string, headBranch: string) => Promise} */ +let listOpenPullRequestsByHeadImpl = async () => []; mock.module('../github-helpers.mjs', { namedExports: { @@ -28,6 +30,8 @@ mock.module('../github-helpers.mjs', { getPrReviews: (n) => getPrReviewsImpl(n), isTeamMember: (slug, username) => isTeamMemberImpl(slug, username), setCommitStatus: (sha, status) => setCommitStatusImpl(sha, status), + listOpenPullRequestsByHead: (headOwner, headBranch) => + listOpenPullRequestsByHeadImpl(headOwner, headBranch), }, }); @@ -46,28 +50,150 @@ mock.module('./owners.mjs', { }); const { + REQUIRED_REVIEW_EXEMPTIONS, STATUS_CONTEXT, buildStatus, collectApprovers, + findExemption, latestReviewStates, + matchesBranchPattern, + parseExemption, resolvePullRequestNumber, run, } = await import('./required-reviews.mjs'); +const REPO = 'n8n-io/n8n'; + +/** @param {string} head @param {string} base @param {string} [headRepo] */ +function pullRequestFor(head, base, headRepo = REPO) { + return { + head: { ref: head, sha: 'head-sha', repo: { full_name: headRepo } }, + base: { ref: base, repo: { full_name: REPO } }, + }; +} + +describe('parseExemption', () => { + it('splits a " -> " route', () => { + assert.deepEqual(parseExemption('sync/master-to-3x -> 3.x'), { + head: 'sync/master-to-3x', + base: '3.x', + source: 'sync/master-to-3x -> 3.x', + }); + }); + + it('treats a bare "" as any head', () => { + assert.deepEqual(parseExemption('release/*'), { head: '*', base: 'release/*', source: 'release/*' }); + }); + + it('rejects malformed routes', () => { + assert.throws(() => parseExemption('a -> b -> c'), /Invalid exemption/); + assert.throws(() => parseExemption(' -> 3.x'), /Invalid exemption/); + assert.throws(() => parseExemption('3.x -> '), /Invalid exemption/); + }); + + it('accepts every configured exemption', () => { + for (const entry of REQUIRED_REVIEW_EXEMPTIONS) parseExemption(entry); + }); +}); + +describe('matchesBranchPattern', () => { + it('matches whole branch names', () => { + assert.equal(matchesBranchPattern('3.x', '3.x'), true); + assert.equal(matchesBranchPattern('3.xy', '3.x'), false); + assert.equal(matchesBranchPattern('v3.x', '3.x'), false); + }); + + it('treats "." as a literal', () => { + assert.equal(matchesBranchPattern('3ax', '3.x'), false); + }); + + it('expands "*" across path separators', () => { + assert.equal(matchesBranchPattern('release/2.39.5', 'release/*'), true); + assert.equal(matchesBranchPattern('release/2.39.5/hotfix', 'release/*'), true); + assert.equal(matchesBranchPattern('anything', '*'), true); + assert.equal(matchesBranchPattern('release-candidate/2.40.0', 'release/*'), false); + }); +}); + +describe('findExemption', () => { + const exemptions = ['sync/master-to-3x -> 3.x', 'release/*']; + + it('finds the route for a matching head and base', () => { + const route = findExemption(pullRequestFor('sync/master-to-3x', '3.x'), exemptions); + assert.equal(route?.source, 'sync/master-to-3x -> 3.x'); + }); + + it('returns nothing when only the base matches', () => { + assert.equal(findExemption(pullRequestFor('feature/x', '3.x'), exemptions), undefined); + }); + + it('returns nothing when only the head matches', () => { + assert.equal(findExemption(pullRequestFor('sync/master-to-3x', 'master'), exemptions), undefined); + }); + + it('matches a bare base route for any head', () => { + const route = findExemption(pullRequestFor('feature/x', 'release/2.39.5'), exemptions); + assert.equal(route?.source, 'release/*'); + }); + + it('ignores heads from another repository', () => { + const fromFork = pullRequestFor('sync/master-to-3x', '3.x', 'someone/n8n'); + assert.equal(findExemption(fromFork, exemptions), undefined); + }); + + it('ignores heads whose repository is gone', () => { + const pullRequest = pullRequestFor('sync/master-to-3x', '3.x'); + pullRequest.head.repo = null; + assert.equal(findExemption(pullRequest, exemptions), undefined); + }); +}); + describe('resolvePullRequestNumber', () => { - it('reads the PR number from pull_request payloads', () => { + /** A workflow_run payload for a run on `owner/branch` at `sha`. */ + function workflowRunEvent(owner, branch, sha) { + return { workflow_run: { head_sha: sha, head_branch: branch, head_repository: { owner: { login: owner } } } }; + } + + beforeEach(() => { + listOpenPullRequestsByHeadImpl = async () => []; + }); + + it('reads the PR number from pull_request payloads', async () => { assert.equal( - resolvePullRequestNumber('pull_request', { pull_request: { number: 42 } }, undefined), + await resolvePullRequestNumber('pull_request_target', { pull_request: { number: 42 } }, undefined), 42, ); }); - it('falls back to the PULL_REQUEST_NUMBER env value for workflow_dispatch', () => { - assert.equal(resolvePullRequestNumber('workflow_dispatch', {}, '7'), 7); + it('falls back to the PULL_REQUEST_NUMBER env value for workflow_dispatch', async () => { + assert.equal(await resolvePullRequestNumber('workflow_dispatch', {}, '7'), 7); }); - it('throws when no source yields a PR number', () => { - assert.throws(() => resolvePullRequestNumber('workflow_dispatch', {}, undefined), /Cannot resolve/); + it('rejects when no source yields a PR number', async () => { + await assert.rejects(resolvePullRequestNumber('workflow_dispatch', {}, undefined), /Cannot resolve/); + }); + + it('looks up the open PR for a workflow_run head', async () => { + const listOpenPullRequestsByHead = mock.fn(async () => [ + { number: 11, head: { sha: 'old-sha' } }, + { number: 12, head: { sha: 'run-sha' } }, + ]); + listOpenPullRequestsByHeadImpl = listOpenPullRequestsByHead; + + assert.equal( + await resolvePullRequestNumber('workflow_run', workflowRunEvent('forker', 'fix/x', 'run-sha'), undefined), + 12, + ); + assert.deepEqual(listOpenPullRequestsByHead.mock.calls[0].arguments, ['forker', 'fix/x']); + }); + + it('returns undefined when no open PR has the workflow_run head SHA', async () => { + listOpenPullRequestsByHeadImpl = async () => [{ number: 11, head: { sha: 'old-sha' } }]; + + assert.equal( + await resolvePullRequestNumber('workflow_run', workflowRunEvent('forker', 'fix/x', 'run-sha'), undefined), + undefined, + ); }); }); @@ -152,8 +278,7 @@ describe('run', () => { eventImpl = () => ({ pull_request: { number: 42 } }); getPullRequestByIdImpl = async () => ({ - base: { ref: 'master' }, - head: { sha: 'head-sha' }, + ...pullRequestFor('feature/x', 'master'), user: { login: 'author' }, }); getChangedFilesImpl = async () => new Set(['a.ts']); @@ -267,15 +392,53 @@ describe('run', () => { assert.equal(status.state, 'pending'); }); - it('skips PRs that do not target master without setting a status', async () => { - getPullRequestByIdImpl = async () => ({ - base: { ref: 'release-candidate/2.9.x' }, - head: { sha: 'head-sha' }, + it('writes no status for a workflow_run head without an open PR', async () => { + process.env.GITHUB_EVENT_NAME = 'workflow_run'; + eventImpl = () => ({ + workflow_run: { head_sha: 'gone-sha', head_branch: 'fix/x', head_repository: { owner: { login: 'forker' } } }, }); + listOpenPullRequestsByHeadImpl = async () => []; await run(); assert.equal(setCommitStatus.mock.calls.length, 0); }); + it('evaluates PRs into branches other than master', async () => { + getPullRequestByIdImpl = async () => pullRequestFor('feature/x', 'release-candidate/2.9.x'); + resolveRequiredTeamsImpl = () => new Map([['@n8n-io/qa-dx', ['a.ts']]]); + + await run(); + + const [, status] = setCommitStatus.mock.calls.at(-1).arguments; + assert.equal(status.state, 'pending'); + assert.match(status.description, /Waiting for approval from: qa-dx/); + }); + + it('reports success without evaluating a PR on an exempt route', async () => { + getPullRequestByIdImpl = async () => pullRequestFor('sync/master-to-3x', '3.x'); + const getChangedFiles = mock.fn(async () => new Set(['a.ts'])); + getChangedFilesImpl = getChangedFiles; + resolveRequiredTeamsImpl = () => new Map([['@n8n-io/qa-dx', ['a.ts']]]); + + await run(); + + assert.equal(getChangedFiles.mock.calls.length, 0); + assert.equal(setCommitStatus.mock.calls.length, 2); + assert.equal(setCommitStatus.mock.calls[0].arguments[1].state, 'pending'); + const [, status] = setCommitStatus.mock.calls[1].arguments; + assert.equal(status.state, 'success'); + assert.equal(status.description, 'Exempt route: sync/master-to-3x -> 3.x'); + }); + + it('does not exempt a fork branch that is named like an exempt route', async () => { + getPullRequestByIdImpl = async () => pullRequestFor('sync/master-to-3x', '3.x', 'someone/n8n'); + resolveRequiredTeamsImpl = () => new Map([['@n8n-io/qa-dx', ['a.ts']]]); + + await run(); + + const [, status] = setCommitStatus.mock.calls.at(-1).arguments; + assert.equal(status.state, 'pending'); + }); + }); diff --git a/.github/workflows/ci-owners-required-reviews.yml b/.github/workflows/ci-owners-required-reviews.yml index 828bef78787..17112e513d8 100644 --- a/.github/workflows/ci-owners-required-reviews.yml +++ b/.github/workflows/ci-owners-required-reviews.yml @@ -8,25 +8,44 @@ run-name: "${{ github.event_name == 'workflow_dispatch' && format('CI: Owners Re # Replaces the GitHub-native CODEOWNERS enforcement, which could not express # team ownership metadata beyond "request a review". # +# Every PR is evaluated, whatever its base branch and whether or not its head +# is a fork. Two PR classes make this necessary: GitHub applies the ruleset of +# a stack's target branch to every PR in the stack, so a PR into a feature +# branch can be gated by the master ruleset; and a fork PR cannot write the +# status from a fork-context run. Every path that reports the status runs +# in the base repository context (secrets, write token). The routes that +# skip the evaluation (for example the master-to-3.x sync branch) live in +# REQUIRED_REVIEW_EXEMPTIONS in required-reviews.mjs; an exempt PR gets a +# success status. +# # Triggers -# - pull_request (opened/synchronize/reopened/ready_for_review): -# re-evaluates whenever the changeset can change. Skipped for fork PRs -# (no secrets): their status stays "expected" — fail closed — until the -# first review event reports it. +# - pull_request_target (opened/synchronize/reopened/ready_for_review, and +# edited when the base branch changes): re-evaluates whenever the +# changeset can change, for same-repo and fork heads alike. A retargeted +# PR keeps its head SHA, so a verdict computed against the old base must +# not survive the retarget. # - pull_request_review (submitted/dismissed): re-evaluates when approvals -# change. A privileged event (base repo context, secrets), so it covers -# fork PRs too — and approvals can only change through review events. +# change on a same-repo PR. A fork PR's review event runs without secrets +# and with a read-only token, so this path skips fork heads. # Comment-only reviews are skipped: they cannot change approvals, and # review bots submit one after every push. +# - workflow_run of "CI: Pull Request Review" (completed): the fork +# counterpart of pull_request_review. That workflow runs on every review +# event; its completion arrives here in the base repository context. The +# PR is looked up from the triggering run's head. Same-repo heads skip +# this path, which pull_request_review already covers. # - merge_group: reports success on the queue head without re-evaluating. # A PR cannot enter the queue unless this status is green on its head, # and the queue does not change approvals. -# - workflow_dispatch: manual re-check (e.g. after a team membership change, -# or a fork PR push after an approval). +# - workflow_dispatch: manual re-check (e.g. after a team membership +# change). # -# The checkout below always pins the base branch, never the PR merge ref: the -# scripts and the OWNERS file are trusted input, so a PR cannot lift its own -# review requirement or reach the status-writing token. +# The checkout below always pins master, never the base branch or the PR +# merge ref: the scripts, the OWNERS file and the exemption routes are trusted +# input, so a PR cannot lift its own review requirement or reach the +# status-writing token. Any branch can be a base, and anyone with push access +# can edit a branch, so only master is trusted. This is also what makes +# pull_request_target safe here: no step checks out or runs PR code. # # Output # - A commit status named "Required Reviews" on the PR head SHA (or @@ -38,12 +57,13 @@ run-name: "${{ github.event_name == 'workflow_dispatch' && format('CI: Owners Re # stale verdict in effect. The job itself only fails on errors. on: - pull_request: - types: [opened, synchronize, reopened, ready_for_review] - branches: - - master + pull_request_target: + types: [opened, synchronize, reopened, ready_for_review, edited] pull_request_review: types: [submitted, dismissed] + workflow_run: + workflows: ['CI: Pull Request Review'] + types: [completed] merge_group: workflow_dispatch: inputs: @@ -90,24 +110,29 @@ jobs: required-reviews: name: Check required reviews - # Fork PRs are skipped on pull_request (no secrets for the app token); - # pull_request_review covers them. pull_request_review has no `branches` - # filter; the script also skips non-master bases, the guard here keeps - # the early pending status off non-master heads. + # Of the edited events only a base change matters; title and body edits + # do not change the verdict. Review events split by head: same-repo + # heads come in through pull_request_review, fork heads through the + # workflow_run of the review workflow. if: >- github.event_name != 'merge_group' && - (github.event_name != 'pull_request' || - github.event.pull_request.head.repo.full_name == github.repository) && + (github.event_name != 'pull_request_target' || + github.event.action != 'edited' || github.event.changes.base) && (github.event_name != 'pull_request_review' || - (github.event.pull_request.base.ref == 'master' && - github.event.review.state != 'commented')) + (github.event.pull_request.head.repo.full_name == github.repository && + github.event.review.state != 'commented')) && + (github.event_name != 'workflow_run' || + (github.event.workflow_run.event == 'pull_request_review' && + github.event.workflow_run.head_repository.full_name != github.repository)) runs-on: ubuntu-latest timeout-minutes: 5 # Job-level, not workflow-level: a run whose job is skipped must not - # cancel an in-flight evaluation, or the status stays pending. + # cancel an in-flight evaluation, or the status stays pending. Keyed by + # head SHA, so the paths above share one group per PR head. concurrency: group: >- - owners-required-reviews-${{ github.event.pull_request.number + owners-required-reviews-${{ github.event.pull_request.head.sha + || github.event.workflow_run.head_sha || inputs.pr_number || github.ref }} cancel-in-progress: true @@ -135,7 +160,7 @@ jobs: if: github.event_name != 'workflow_dispatch' env: GH_TOKEN: ${{ steps.app-token.outputs.token }} - SHA: ${{ github.event.pull_request.head.sha }} + SHA: ${{ github.event.pull_request.head.sha || github.event.workflow_run.head_sha }} RUN_URL: ${{ github.server_url }}/${{ github.repository }}/actions/runs/${{ github.run_id }} run: | gh api "repos/${GITHUB_REPOSITORY}/statuses/${SHA}" \ @@ -144,44 +169,17 @@ jobs: -f description='Evaluating required reviews' \ -f "target_url=${RUN_URL}" - # Always check out the base branch, never the PR merge ref: the OWNERS - # file and the scripts must be trusted input. The 'master' fallback - # covers workflow_dispatch, so a dispatch from another branch cannot - # run that branch's policy. The script reads only `.github/` and the - # root files, which cone mode always includes. + # Always check out master, never the base branch or the PR merge ref: + # the OWNERS file, the scripts and the exemption routes must be trusted + # input, and any writable branch can be a base. The script reads only + # `.github/` and the root files, which cone mode always includes. - uses: actions/checkout@de0fac2e4500dabe0009e67214ff5f5447ce83dd # v6.0.2 with: - ref: ${{ github.event.pull_request.base.ref || 'master' }} + ref: master persist-credentials: false sparse-checkout: .github - # Bootstrap: the checkout is the trusted base branch. Until the action - # exists there, there is nothing to run. Report success so the PR that - # introduces it can merge; the check is enforced again from the next PR. - - name: Check for the enforcement action on the base branch - id: bootstrap - shell: bash - run: | - if [ -f .github/actions/run-workflow-script/action.yml ]; then - echo "ready=true" >> "$GITHUB_OUTPUT" - else - echo "ready=false" >> "$GITHUB_OUTPUT" - echo "No enforcement action on the base branch; nothing to enforce." - fi - - - name: Report the bootstrap state - if: steps.bootstrap.outputs.ready != 'true' && github.event.pull_request.head.sha != '' - env: - GH_TOKEN: ${{ steps.app-token.outputs.token }} - SHA: ${{ github.event.pull_request.head.sha }} - run: | - gh api "repos/${GITHUB_REPOSITORY}/statuses/${SHA}" \ - -f state=success \ - -f context='Required Reviews' \ - -f description='Owners enforcement is not on the base branch yet' - - name: Evaluate required reviews - if: steps.bootstrap.outputs.ready == 'true' uses: ./.github/actions/run-workflow-script env: PULL_REQUEST_NUMBER: ${{ inputs.pr_number || '' }} diff --git a/.github/workflows/ci-pull-request-review.yml b/.github/workflows/ci-pull-request-review.yml index 8692e6663d7..2c9ca0ed9d8 100644 --- a/.github/workflows/ci-pull-request-review.yml +++ b/.github/workflows/ci-pull-request-review.yml @@ -1,8 +1,11 @@ name: 'CI: Pull Request Review' +# "CI: Owners Required Reviews" listens to this workflow's completion +# (workflow_run) to re-evaluate fork PRs: a fork's review event runs without +# secrets, a workflow_run does not. Keep the trigger on every review event. on: pull_request_review: - types: [submitted] + types: [submitted, dismissed] concurrency: group: ci-review-${{ github.event.pull_request.number }} diff --git a/packages/testing/code-health/src/index.ts b/packages/testing/code-health/src/index.ts index 4412366e880..2b52fa20bd8 100644 --- a/packages/testing/code-health/src/index.ts +++ b/packages/testing/code-health/src/index.ts @@ -35,7 +35,8 @@ const defaultRuleSettings: RuleSettingsMap = { 'workflow-pr-target-safety': { enabled: true, severity: 'error', - options: { allowedWorkflows: ['ci-cla-check.yml'] }, + // ci-owners-required-reviews.yml checks out master only, never PR code. + options: { allowedWorkflows: ['ci-cla-check.yml', 'ci-owners-required-reviews.yml'] }, }, 'migration-timestamp': { enabled: true,