From 77443d328a1c398b2ff8e093b8a90afa81125dc0 Mon Sep 17 00:00:00 2001 From: "Felipe R. de Almeida" Date: Thu, 1 Oct 2026 15:56:01 -0300 Subject: [PATCH 1/5] feat(ci): add release approval guard action --- .../actions/release-approval-guard/README.md | 35 ++++ .../release-approval-guard/action.yaml | 27 +++ .../actions/release-approval-guard/guard.mjs | 192 ++++++++++++++++++ .../release-approval-guard/guard.test.mjs | 151 ++++++++++++++ .../release-approval-guard-tests.yml | 24 +++ AGENTS.md | 6 + 6 files changed, 435 insertions(+) create mode 100644 .github/actions/release-approval-guard/README.md create mode 100644 .github/actions/release-approval-guard/action.yaml create mode 100644 .github/actions/release-approval-guard/guard.mjs create mode 100644 .github/actions/release-approval-guard/guard.test.mjs create mode 100644 .github/workflows/release-approval-guard-tests.yml diff --git a/.github/actions/release-approval-guard/README.md b/.github/actions/release-approval-guard/README.md new file mode 100644 index 0000000..e9fb2a6 --- /dev/null +++ b/.github/actions/release-approval-guard/README.md @@ -0,0 +1,35 @@ +# Release approval guard + +Fails a release when someone who approved its environment authored, or pushed commits to, a merged PR whose changeset is in the release. + +GitHub's "Prevent self-review" environment setting compares the approver only with the run's triggering actor. With a merge queue (GitHub or Trunk), that actor is a bot, so a PR author can approve their own release. The setting also never checked the other authors in a release that batches several PRs. + +## Usage + +Add the step to the job that uses the approval environment, after the checkout of the release ref and before the job uses its secrets: + +```yaml +jobs: + version-bump: + environment: 'NPM Release' + permissions: + contents: read + actions: read # approvals of this run + pull-requests: read # PRs and their commits + steps: + - uses: actions/checkout@ + - uses: PostHog/.github/.github/actions/release-approval-guard@ + with: + environment: 'NPM Release' + changeset-dirs: '.changeset' # e.g. cli/.sampo/changesets for sampo +``` + +The checkout can be shallow. The step reads the changeset files in the working tree and resolves their history through the GitHub API at `HEAD`. It needs Node, which GitHub-hosted runners include. + +## Behavior + +- Contributors are the author of every merged PR that added or edited a pending changeset, plus the author and committer of every commit in those PRs. +- It fails closed. It fails if there are no changesets, if the run has no approval for the environment, or if a commit that changed a changeset isn't part of a merged PR. +- The approvals API doesn't say which run attempt an approval belongs to, so every approval in the run counts. After a block, a different approver must approve a **new run**. A re-run can still see the blocked approval. + +Run the tests with `node --test .github/actions/release-approval-guard/guard.test.mjs`. diff --git a/.github/actions/release-approval-guard/action.yaml b/.github/actions/release-approval-guard/action.yaml new file mode 100644 index 0000000..6c42199 --- /dev/null +++ b/.github/actions/release-approval-guard/action.yaml @@ -0,0 +1,27 @@ +name: 'Release approval guard' +description: 'Fail a release when an approver of its environment authored or committed to a PR in the release' + +inputs: + environment: + description: 'Name of the environment whose approval gates the release, e.g. "NPM Release"' + required: true + changeset-dirs: + description: 'Newline-separated directories, relative to the checkout, that hold the pending changeset .md files' + required: false + default: '.changeset' + github-token: + description: 'Token with `actions: read`, `contents: read` and `pull-requests: read` on the repository' + required: false + default: ${{ github.token }} + +runs: + using: 'composite' + steps: + - name: Check release approvers against release contributors + shell: bash + env: + GH_TOKEN: ${{ inputs.github-token }} + ENVIRONMENT: ${{ inputs.environment }} + CHANGESET_DIRS: ${{ inputs.changeset-dirs }} + ACTION_PATH: ${{ github.action_path }} + run: node "$ACTION_PATH/guard.mjs" diff --git a/.github/actions/release-approval-guard/guard.mjs b/.github/actions/release-approval-guard/guard.mjs new file mode 100644 index 0000000..5ba2f8b --- /dev/null +++ b/.github/actions/release-approval-guard/guard.mjs @@ -0,0 +1,192 @@ +#!/usr/bin/env node +// Release approval guard, run from PostHog/.github/.github/actions/release-approval-guard. +// +// Fails when a person who approved the release environment in this workflow run +// authored, or committed to, a merged PR whose changeset is part of the release. +// +// GitHub's "prevent self-review" environment setting only compares the approver +// with the run's triggering actor. When a merge queue (GitHub or Trunk) merges, +// that actor is a bot, so the PR author can approve their own release. A +// release can also batch changesets from several PRs, and the setting never +// checked those other authors. +// +// Fails closed: any changeset or commit it cannot attribute to a merged PR, or +// a run without an approval for the environment, blocks the release. + +import { execFileSync } from 'node:child_process'; +import { appendFileSync, existsSync, readdirSync } from 'node:fs'; +import { join } from 'node:path'; +import { fileURLToPath } from 'node:url'; + +// GitHub sets this committer for commits made in the web UI and for squash merges. +const IGNORED_LOGINS = new Set(['web-flow']); + +export class GuardError extends Error {} + +// Pending changeset files (*.md, except README.md) in the given directories. +export function listChangesets(dirs, cwd = process.cwd()) { + const files = []; + for (const dir of dirs) { + if (!existsSync(join(cwd, dir))) continue; + for (const name of readdirSync(join(cwd, dir))) { + if (name.endsWith('.md') && name.toLowerCase() !== 'readme.md') { + files.push(`${dir.replace(/\/+$/, '')}/${name}`); + } + } + } + return files.sort(); +} + +export function createApi({ token, apiUrl = 'https://api.github.com', fetchImpl = fetch }) { + async function request(url) { + const res = await fetchImpl(url, { + headers: { + Accept: 'application/vnd.github+json', + Authorization: `Bearer ${token}`, + 'X-GitHub-Api-Version': '2022-11-28', + }, + }); + if (!res.ok) { + throw new GuardError(`GitHub API ${res.status} for ${url}: ${await res.text()}`); + } + return res; + } + + return { + async get(path) { + return (await request(`${apiUrl}${path}`)).json(); + }, + // Follows Link: rel="next" for list endpoints. + async list(path) { + const items = []; + let url = `${apiUrl}${path}`; + while (url) { + const res = await request(url); + items.push(...(await res.json())); + url = res.headers.get('link')?.match(/<([^>]+)>;\s*rel="next"/)?.[1]; + } + return items; + }, + }; +} + +// Logins that approved `environment` in this run. The API does not say which +// run attempt an approval belongs to, so every approval in the run counts. +export async function approversFor(api, repo, runId, environment) { + const approvals = await api.get(`/repos/${repo}/actions/runs/${runId}/approvals`); + const logins = approvals + .filter((a) => a.state === 'approved' && a.environments.some((e) => e.name === environment)) + .map((a) => a.user.login); + return [...new Set(logins)].sort(); +} + +// login (lowercased) -> { login, reasons: Set } for everyone who authored or +// committed to a merged PR that touched one of `files` up to `ref`. +export async function contributorsFor(api, repo, ref, files) { + const contributors = new Map(); + const add = (login, reason) => { + if (!login || IGNORED_LOGINS.has(login)) return; + const key = login.toLowerCase(); + if (!contributors.has(key)) contributors.set(key, { login, reasons: new Set() }); + contributors.get(key).reasons.add(reason); + }; + + const seenPrs = new Set(); + for (const file of files) { + const commits = await api.list( + `/repos/${repo}/commits?sha=${encodeURIComponent(ref)}&path=${encodeURIComponent(file)}&per_page=100`, + ); + if (commits.length === 0) { + throw new GuardError(`${file} has no commits at ${ref}. Is it committed?`); + } + for (const commit of commits) { + const prs = (await api.get(`/repos/${repo}/commits/${commit.sha}/pulls`)).filter( + (pr) => pr.merged_at, + ); + if (prs.length === 0) { + throw new GuardError( + `Commit ${commit.sha} changed ${file} but does not belong to a merged PR.`, + ); + } + for (const pr of prs) { + if (seenPrs.has(pr.number)) continue; + seenPrs.add(pr.number); + add(pr.user?.login, `author of #${pr.number}`); + for (const c of await api.list(`/repos/${repo}/pulls/${pr.number}/commits?per_page=100`)) { + add(c.author?.login, `commit in #${pr.number}`); + add(c.committer?.login, `commit in #${pr.number}`); + } + } + } + } + return contributors; +} + +export async function checkReleaseApproval({ api, repo, runId, environment, ref, files }) { + if (files.length === 0) { + throw new GuardError('No changesets found. Check the changeset-dirs input.'); + } + const approvers = await approversFor(api, repo, runId, environment); + if (approvers.length === 0) { + throw new GuardError( + `No approval for the "${environment}" environment in this run. ` + + 'Check the environment input and that the environment has required reviewers.', + ); + } + const contributors = await contributorsFor(api, repo, ref, files); + const violations = approvers + .filter((login) => contributors.has(login.toLowerCase())) + .map((login) => ({ login, reasons: [...contributors.get(login.toLowerCase()).reasons] })); + return { approvers, contributors: [...contributors.values()].map((c) => c.login).sort(), violations }; +} + +async function main() { + const env = process.env; + const environment = env.ENVIRONMENT?.trim(); + if (!environment) throw new GuardError('The environment input is required.'); + const dirs = (env.CHANGESET_DIRS || '.changeset') + .split('\n') + .map((d) => d.trim()) + .filter(Boolean); + const ref = execFileSync('git', ['rev-parse', 'HEAD'], { encoding: 'utf8' }).trim(); + const files = listChangesets(dirs); + + const result = await checkReleaseApproval({ + api: createApi({ token: env.GH_TOKEN, apiUrl: env.GITHUB_API_URL }), + repo: env.GITHUB_REPOSITORY, + runId: env.GITHUB_RUN_ID, + environment, + ref, + files, + }); + + console.log(`Changesets at ${ref}:\n ${files.join('\n ')}`); + console.log(`Approvers of "${environment}": ${result.approvers.join(', ')}`); + console.log(`Contributors to released PRs: ${result.contributors.join(', ')}`); + + if (result.violations.length > 0) { + const lines = result.violations.map((v) => `${v.login} (${v.reasons.join(', ')})`); + console.log( + `::error title=Release approved by a contributor::${lines.join('; ')} approved a release ` + + 'that contains their own changes. Another approver must approve a new run of this ' + + 'workflow. A re-run of this run can still see the earlier approval and fail again.', + ); + if (env.GITHUB_STEP_SUMMARY) { + appendFileSync( + env.GITHUB_STEP_SUMMARY, + `### Release blocked\n\nApproved by a contributor to this release:\n\n${lines + .map((l) => `- ${l}`) + .join('\n')}\n`, + ); + } + process.exit(1); + } + console.log('✓ No approver contributed to the released changes.'); +} + +if (process.argv[1] === fileURLToPath(import.meta.url)) { + main().catch((e) => { + console.log(`::error title=Release approval guard::${e.message}`); + process.exit(1); + }); +} diff --git a/.github/actions/release-approval-guard/guard.test.mjs b/.github/actions/release-approval-guard/guard.test.mjs new file mode 100644 index 0000000..7737717 --- /dev/null +++ b/.github/actions/release-approval-guard/guard.test.mjs @@ -0,0 +1,151 @@ +import assert from 'node:assert/strict'; +import { mkdirSync, mkdtempSync, rmSync, writeFileSync } from 'node:fs'; +import { tmpdir } from 'node:os'; +import { join } from 'node:path'; +import test from 'node:test'; + +import { checkReleaseApproval, createApi, GuardError, listChangesets } from './guard.mjs'; + +const repo = 'PostHog/posthog-js'; +const ref = 'head-sha'; +const API = 'https://api.github.test'; + +// Fake GitHub API: `routes` maps a path (with query) to a JSON body, or to an +// array of pages for paginated list endpoints. +function fakeApi(routes) { + const fetchImpl = async (url) => { + const path = url.slice(API.length); + const [route, page] = path.split('&page='); + if (!(route in routes)) return new Response(`no route ${path}`, { status: 404 }); + const body = routes[route]; + if (Array.isArray(body) && Array.isArray(body[0])) { + const n = Number(page || 1); + const headers = n < body.length ? { link: `<${API}${route}&page=${n + 1}>; rel="next"` } : {}; + return Response.json(body[n - 1], { headers }); + } + return Response.json(body); + }; + return createApi({ token: 't', apiUrl: API, fetchImpl }); +} + +const approval = (login, environment = 'NPM Release', state = 'approved') => ({ + state, + user: { login }, + environments: [{ name: environment }], +}); +const commitsFor = (file) => `/repos/${repo}/commits?sha=${ref}&path=${encodeURIComponent(file)}&per_page=100`; +const pullsFor = (sha) => `/repos/${repo}/commits/${sha}/pulls`; +const prCommits = (n) => `/repos/${repo}/pulls/${n}/commits?per_page=100`; +const pr = (number, login) => ({ number, merged_at: '2026-10-01T00:00:00Z', user: { login } }); +const commit = (author, committer = 'web-flow') => ({ author: { login: author }, committer: { login: committer } }); + +// One changeset, added by PR #1 from `author`, with the given approvals. +function singlePr({ author = 'alice', approvals = [approval('bob')], commits = [commit(author)] } = {}) { + return { + [`/repos/${repo}/actions/runs/7/approvals`]: approvals, + [commitsFor('.changeset/a.md')]: [{ sha: 'c1' }], + [pullsFor('c1')]: [pr(1, author)], + [prCommits(1)]: commits, + }; +} + +const check = (routes, files = ['.changeset/a.md'], environment = 'NPM Release') => + checkReleaseApproval({ api: fakeApi(routes), repo, runId: 7, environment, ref, files }); + +test('passes when no approver contributed to the release', async () => { + const result = await check(singlePr()); + assert.deepEqual(result.violations, []); + assert.deepEqual(result.approvers, ['bob']); + assert.deepEqual(result.contributors, ['alice']); +}); + +test('blocks the PR author approving their own release', async () => { + const result = await check(singlePr({ approvals: [approval('alice')] })); + assert.deepEqual(result.violations, [{ login: 'alice', reasons: ['author of #1', 'commit in #1'] }]); +}); + +test('blocks someone who pushed commits to another person\'s PR', async () => { + const result = await check(singlePr({ commits: [commit('alice'), commit('bob', 'bob')] })); + assert.deepEqual(result.violations, [{ login: 'bob', reasons: ['commit in #1'] }]); +}); + +test('matches logins case-insensitively', async () => { + const result = await check(singlePr({ approvals: [approval('Alice')] })); + assert.equal(result.violations[0].login, 'Alice'); +}); + +test('blocks when any approval in the run came from a contributor', async () => { + const result = await check(singlePr({ approvals: [approval('bob'), approval('alice')] })); + assert.deepEqual(result.violations.map((v) => v.login), ['alice']); +}); + +test('checks every PR in a batched release, including PRs that edited a changeset', async () => { + const routes = { + [`/repos/${repo}/actions/runs/7/approvals`]: [approval('carol')], + [commitsFor('.changeset/a.md')]: [{ sha: 'c1' }], + [commitsFor('.changeset/b.md')]: [{ sha: 'c3' }, { sha: 'c2' }], + [pullsFor('c1')]: [pr(1, 'alice')], + [pullsFor('c2')]: [pr(2, 'bob')], + [pullsFor('c3')]: [pr(3, 'carol')], + [prCommits(1)]: [commit('alice')], + [prCommits(2)]: [commit('bob')], + [prCommits(3)]: [commit('carol')], + }; + const result = await check(routes, ['.changeset/a.md', '.changeset/b.md']); + assert.deepEqual(result.contributors, ['alice', 'bob', 'carol']); + assert.deepEqual(result.violations.map((v) => v.login), ['carol']); +}); + +test('follows pagination of PR commits', async () => { + const routes = singlePr(); + routes[prCommits(1)] = [[commit('alice')], [commit('bob')]]; + const result = await check(routes); + assert.deepEqual(result.violations.map((v) => v.login), ['bob']); +}); + +test('ignores approvals that are rejected or for other environments', async () => { + const approvals = [approval('alice', 'S3 Upload'), approval('alice', 'NPM Release', 'rejected'), approval('bob')]; + const result = await check(singlePr({ approvals })); + assert.deepEqual(result.approvers, ['bob']); + assert.deepEqual(result.violations, []); +}); + +test('fails closed when the run has no approval for the environment', async () => { + await assert.rejects(check(singlePr({ approvals: [approval('bob', 'Other')] })), GuardError); +}); + +test('fails closed when a changeset commit is not from a merged PR', async () => { + const routes = singlePr(); + routes[pullsFor('c1')] = [{ ...pr(1, 'alice'), merged_at: null }]; + await assert.rejects(check(routes), /does not belong to a merged PR/); +}); + +test('fails closed when a changeset has no commits', async () => { + const routes = singlePr(); + routes[commitsFor('.changeset/a.md')] = []; + await assert.rejects(check(routes), /has no commits/); +}); + +test('fails closed when there are no changesets', async () => { + await assert.rejects(check(singlePr(), []), /No changesets found/); +}); + +test('surfaces GitHub API errors', async () => { + const routes = singlePr(); + delete routes[prCommits(1)]; + await assert.rejects(check(routes), /GitHub API 404/); +}); + +test('lists pending changesets across directories', (t) => { + const cwd = mkdtempSync(join(tmpdir(), 'release-approval-guard-')); + t.after(() => rmSync(cwd, { recursive: true, force: true })); + for (const file of ['.changeset/b.md', '.changeset/a.md', '.changeset/README.md', '.changeset/config.json', 'cli/.sampo/changesets/c.md']) { + mkdirSync(join(cwd, file, '..'), { recursive: true }); + writeFileSync(join(cwd, file), ''); + } + assert.deepEqual(listChangesets(['.changeset', 'cli/.sampo/changesets/', 'missing'], cwd), [ + '.changeset/a.md', + '.changeset/b.md', + 'cli/.sampo/changesets/c.md', + ]); +}); diff --git a/.github/workflows/release-approval-guard-tests.yml b/.github/workflows/release-approval-guard-tests.yml new file mode 100644 index 0000000..ab2f868 --- /dev/null +++ b/.github/workflows/release-approval-guard-tests.yml @@ -0,0 +1,24 @@ +name: Release approval guard tests + +on: + pull_request: + merge_group: + +permissions: + contents: read + +jobs: + test: + runs-on: ubuntu-latest + timeout-minutes: 5 + steps: + - name: Checkout + uses: actions/checkout@de0fac2e4500dabe0009e67214ff5f5447ce83dd # v6.0.2 + + - name: Setup Node + uses: actions/setup-node@48b55a011bda9f5d6aeb4c2d9c7362e8dae4041e # v6.4.0 + with: + node-version: '22' + + - name: Test release approval guard + run: node --test .github/actions/release-approval-guard/guard.test.mjs diff --git a/AGENTS.md b/AGENTS.md index 4e1cfe6..cbfff82 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -24,6 +24,12 @@ No general test suite. Run the changeset hygiene script tests with: node --test .github/scripts/check-changeset-coverage.test.mjs ``` +Run the release approval guard tests with: + +```bash +node --test .github/actions/release-approval-guard/guard.test.mjs +``` + The semgrep rule tests run with: ```bash From f4a6d2fd0b83552ecf053c2de842b95ad6185243 Mon Sep 17 00:00:00 2001 From: "Felipe R. de Almeida" Date: Thu, 1 Oct 2026 16:01:41 -0300 Subject: [PATCH 2/5] chore(ci): trim comments in release approval guard --- .../actions/release-approval-guard/guard.mjs | 23 ++----------------- .../release-approval-guard/guard.test.mjs | 3 --- 2 files changed, 2 insertions(+), 24 deletions(-) diff --git a/.github/actions/release-approval-guard/guard.mjs b/.github/actions/release-approval-guard/guard.mjs index 5ba2f8b..82f0765 100644 --- a/.github/actions/release-approval-guard/guard.mjs +++ b/.github/actions/release-approval-guard/guard.mjs @@ -1,29 +1,14 @@ #!/usr/bin/env node -// Release approval guard, run from PostHog/.github/.github/actions/release-approval-guard. -// -// Fails when a person who approved the release environment in this workflow run -// authored, or committed to, a merged PR whose changeset is part of the release. -// -// GitHub's "prevent self-review" environment setting only compares the approver -// with the run's triggering actor. When a merge queue (GitHub or Trunk) merges, -// that actor is a bot, so the PR author can approve their own release. A -// release can also batch changesets from several PRs, and the setting never -// checked those other authors. -// -// Fails closed: any changeset or commit it cannot attribute to a merged PR, or -// a run without an approval for the environment, blocks the release. - import { execFileSync } from 'node:child_process'; import { appendFileSync, existsSync, readdirSync } from 'node:fs'; import { join } from 'node:path'; import { fileURLToPath } from 'node:url'; -// GitHub sets this committer for commits made in the web UI and for squash merges. +// GitHub's committer for web UI commits and squash merges. const IGNORED_LOGINS = new Set(['web-flow']); export class GuardError extends Error {} -// Pending changeset files (*.md, except README.md) in the given directories. export function listChangesets(dirs, cwd = process.cwd()) { const files = []; for (const dir of dirs) { @@ -56,7 +41,6 @@ export function createApi({ token, apiUrl = 'https://api.github.com', fetchImpl async get(path) { return (await request(`${apiUrl}${path}`)).json(); }, - // Follows Link: rel="next" for list endpoints. async list(path) { const items = []; let url = `${apiUrl}${path}`; @@ -70,8 +54,7 @@ export function createApi({ token, apiUrl = 'https://api.github.com', fetchImpl }; } -// Logins that approved `environment` in this run. The API does not say which -// run attempt an approval belongs to, so every approval in the run counts. +// Approvals aren't tied to a run attempt, so every approval in the run counts. export async function approversFor(api, repo, runId, environment) { const approvals = await api.get(`/repos/${repo}/actions/runs/${runId}/approvals`); const logins = approvals @@ -80,8 +63,6 @@ export async function approversFor(api, repo, runId, environment) { return [...new Set(logins)].sort(); } -// login (lowercased) -> { login, reasons: Set } for everyone who authored or -// committed to a merged PR that touched one of `files` up to `ref`. export async function contributorsFor(api, repo, ref, files) { const contributors = new Map(); const add = (login, reason) => { diff --git a/.github/actions/release-approval-guard/guard.test.mjs b/.github/actions/release-approval-guard/guard.test.mjs index 7737717..6d621a3 100644 --- a/.github/actions/release-approval-guard/guard.test.mjs +++ b/.github/actions/release-approval-guard/guard.test.mjs @@ -10,8 +10,6 @@ const repo = 'PostHog/posthog-js'; const ref = 'head-sha'; const API = 'https://api.github.test'; -// Fake GitHub API: `routes` maps a path (with query) to a JSON body, or to an -// array of pages for paginated list endpoints. function fakeApi(routes) { const fetchImpl = async (url) => { const path = url.slice(API.length); @@ -39,7 +37,6 @@ const prCommits = (n) => `/repos/${repo}/pulls/${n}/commits?per_page=100`; const pr = (number, login) => ({ number, merged_at: '2026-10-01T00:00:00Z', user: { login } }); const commit = (author, committer = 'web-flow') => ({ author: { login: author }, committer: { login: committer } }); -// One changeset, added by PR #1 from `author`, with the given approvals. function singlePr({ author = 'alice', approvals = [approval('bob')], commits = [commit(author)] } = {}) { return { [`/repos/${repo}/actions/runs/7/approvals`]: approvals, From bcaba68fce1ff4776da2adee340c0e4cd686ba71 Mon Sep 17 00:00:00 2001 From: "Felipe R. de Almeida" Date: Thu, 1 Oct 2026 16:02:51 -0300 Subject: [PATCH 3/5] ci: run release approval guard tests on node 24 --- .github/workflows/release-approval-guard-tests.yml | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/.github/workflows/release-approval-guard-tests.yml b/.github/workflows/release-approval-guard-tests.yml index ab2f868..2b95e32 100644 --- a/.github/workflows/release-approval-guard-tests.yml +++ b/.github/workflows/release-approval-guard-tests.yml @@ -18,7 +18,7 @@ jobs: - name: Setup Node uses: actions/setup-node@48b55a011bda9f5d6aeb4c2d9c7362e8dae4041e # v6.4.0 with: - node-version: '22' + node-version: '24' - name: Test release approval guard run: node --test .github/actions/release-approval-guard/guard.test.mjs From 6d006c059686116212a555162d7ff0ce0e18f179 Mon Sep 17 00:00:00 2001 From: "Felipe R. de Almeida" Date: Thu, 1 Oct 2026 16:31:12 -0300 Subject: [PATCH 4/5] fix(ci): fail release approval guard on unverified or unlisted PR commits --- .../actions/release-approval-guard/README.md | 3 ++- .../actions/release-approval-guard/guard.mjs | 20 +++++++++++++++++-- .../release-approval-guard/guard.test.mjs | 18 ++++++++++++++++- 3 files changed, 37 insertions(+), 4 deletions(-) diff --git a/.github/actions/release-approval-guard/README.md b/.github/actions/release-approval-guard/README.md index e9fb2a6..f4e5ddb 100644 --- a/.github/actions/release-approval-guard/README.md +++ b/.github/actions/release-approval-guard/README.md @@ -29,7 +29,8 @@ The checkout can be shallow. The step reads the changeset files in the working t ## Behavior - Contributors are the author of every merged PR that added or edited a pending changeset, plus the author and committer of every commit in those PRs. -- It fails closed. It fails if there are no changesets, if the run has no approval for the environment, or if a commit that changed a changeset isn't part of a merged PR. +- It fails closed. It fails if there are no changesets, if the run has no approval for the environment, if a commit that changed a changeset isn't part of a merged PR, if a PR has 250 or more commits (the most GitHub lists for a PR), or if a PR has a commit without a verified signature. +- Commit author and committer emails can be set to anything. A verified signature ties the committer to the GitHub account that owns the signing key, so the guard only trusts signed commits. The org's "Require signed commits" ruleset enforces signing on branches in our repos, but not on forks. - The approvals API doesn't say which run attempt an approval belongs to, so every approval in the run counts. After a block, a different approver must approve a **new run**. A re-run can still see the blocked approval. Run the tests with `node --test .github/actions/release-approval-guard/guard.test.mjs`. diff --git a/.github/actions/release-approval-guard/guard.mjs b/.github/actions/release-approval-guard/guard.mjs index 82f0765..ba77c60 100644 --- a/.github/actions/release-approval-guard/guard.mjs +++ b/.github/actions/release-approval-guard/guard.mjs @@ -6,6 +6,7 @@ import { fileURLToPath } from 'node:url'; // GitHub's committer for web UI commits and squash merges. const IGNORED_LOGINS = new Set(['web-flow']); +const PR_COMMITS_LIMIT = 250; export class GuardError extends Error {} @@ -93,7 +94,18 @@ export async function contributorsFor(api, repo, ref, files) { if (seenPrs.has(pr.number)) continue; seenPrs.add(pr.number); add(pr.user?.login, `author of #${pr.number}`); - for (const c of await api.list(`/repos/${repo}/pulls/${pr.number}/commits?per_page=100`)) { + const prCommits = await api.list(`/repos/${repo}/pulls/${pr.number}/commits?per_page=100`); + if (prCommits.length >= PR_COMMITS_LIMIT) { + throw new GuardError( + `#${pr.number} has ${PR_COMMITS_LIMIT} or more commits, the most GitHub lists, so its contributors cannot all be checked.`, + ); + } + for (const c of prCommits) { + if (!c.commit?.verification?.verified) { + throw new GuardError( + `Commit ${c.sha} in #${pr.number} has no verified signature, so its committer cannot be trusted.`, + ); + } add(c.author?.login, `commit in #${pr.number}`); add(c.committer?.login, `commit in #${pr.number}`); } @@ -118,7 +130,11 @@ export async function checkReleaseApproval({ api, repo, runId, environment, ref, const violations = approvers .filter((login) => contributors.has(login.toLowerCase())) .map((login) => ({ login, reasons: [...contributors.get(login.toLowerCase()).reasons] })); - return { approvers, contributors: [...contributors.values()].map((c) => c.login).sort(), violations }; + return { + approvers, + contributors: [...contributors.values()].map((c) => c.login).sort(), + violations, + }; } async function main() { diff --git a/.github/actions/release-approval-guard/guard.test.mjs b/.github/actions/release-approval-guard/guard.test.mjs index 6d621a3..5b0e2ef 100644 --- a/.github/actions/release-approval-guard/guard.test.mjs +++ b/.github/actions/release-approval-guard/guard.test.mjs @@ -35,7 +35,12 @@ const commitsFor = (file) => `/repos/${repo}/commits?sha=${ref}&path=${encodeURI const pullsFor = (sha) => `/repos/${repo}/commits/${sha}/pulls`; const prCommits = (n) => `/repos/${repo}/pulls/${n}/commits?per_page=100`; const pr = (number, login) => ({ number, merged_at: '2026-10-01T00:00:00Z', user: { login } }); -const commit = (author, committer = 'web-flow') => ({ author: { login: author }, committer: { login: committer } }); +const commit = (author, committer = 'web-flow', verified = true) => ({ + sha: 'abcdef0123', + author: author && { login: author }, + committer: committer && { login: committer }, + commit: { verification: { verified } }, +}); function singlePr({ author = 'alice', approvals = [approval('bob')], commits = [commit(author)] } = {}) { return { @@ -93,6 +98,17 @@ test('checks every PR in a batched release, including PRs that edited a changese assert.deepEqual(result.violations.map((v) => v.login), ['carol']); }); +test('fails closed when a PR has a commit without a verified signature', async () => { + const routes = singlePr({ commits: [commit('alice'), commit('bob', 'bob', false)] }); + await assert.rejects(check(routes), /abcdef0123 in #1 has no verified signature/); +}); + +test('fails closed when a PR reaches the 250-commit listing limit', async () => { + const routes = singlePr(); + routes[prCommits(1)] = [Array(100).fill(commit('alice')), Array(100).fill(commit('alice')), Array(50).fill(commit('alice'))]; + await assert.rejects(check(routes), /#1 has 250 or more commits/); +}); + test('follows pagination of PR commits', async () => { const routes = singlePr(); routes[prCommits(1)] = [[commit('alice')], [commit('bob')]]; From 57b2a0790de238acf638ae71cb91813a896ab1d5 Mon Sep 17 00:00:00 2001 From: "Felipe R. de Almeida" Date: Fri, 2 Oct 2026 15:05:30 -0300 Subject: [PATCH 5/5] refactor(ci): check authors and mergers of the triggering push instead of changesets --- .../actions/release-approval-guard/README.md | 20 +- .../release-approval-guard/action.yaml | 9 +- .../actions/release-approval-guard/guard.mjs | 179 ++++++---------- .../release-approval-guard/guard.test.mjs | 195 ++++++++---------- 4 files changed, 162 insertions(+), 241 deletions(-) diff --git a/.github/actions/release-approval-guard/README.md b/.github/actions/release-approval-guard/README.md index f4e5ddb..35ccf50 100644 --- a/.github/actions/release-approval-guard/README.md +++ b/.github/actions/release-approval-guard/README.md @@ -1,12 +1,12 @@ # Release approval guard -Fails a release when someone who approved its environment authored, or pushed commits to, a merged PR whose changeset is in the release. +Fails a release when someone who approved its environment authored or merged a PR in the push that triggered the run. -GitHub's "Prevent self-review" environment setting compares the approver only with the run's triggering actor. With a merge queue (GitHub or Trunk), that actor is a bot, so a PR author can approve their own release. The setting also never checked the other authors in a release that batches several PRs. +This restores what GitHub's "Prevent self-review" environment setting did before merge queues. That setting excludes the run's triggering actor. When a person merged a PR, the actor was that person, so they couldn't approve the release. With a merge queue (GitHub or Trunk), the actor is a bot, so nobody is excluded. ## Usage -Add the step to the job that uses the approval environment, after the checkout of the release ref and before the job uses its secrets: +Add the step to the job that uses the approval environment, before the job uses its secrets: ```yaml jobs: @@ -15,22 +15,22 @@ jobs: permissions: contents: read actions: read # approvals of this run - pull-requests: read # PRs and their commits + pull-requests: read # PRs in the push steps: - - uses: actions/checkout@ - uses: PostHog/.github/.github/actions/release-approval-guard@ with: environment: 'NPM Release' - changeset-dirs: '.changeset' # e.g. cli/.sampo/changesets for sampo ``` -The checkout can be shallow. The step reads the changeset files in the working tree and resolves their history through the GitHub API at `HEAD`. It needs Node, which GitHub-hosted runners include. +It needs no checkout. It needs Node, which GitHub-hosted runners include. ## Behavior -- Contributors are the author of every merged PR that added or edited a pending changeset, plus the author and committer of every commit in those PRs. -- It fails closed. It fails if there are no changesets, if the run has no approval for the environment, if a commit that changed a changeset isn't part of a merged PR, if a PR has 250 or more commits (the most GitHub lists for a PR), or if a PR has a commit without a verified signature. -- Commit author and committer emails can be set to anything. A verified signature ties the committer to the GitHub account that owns the signing key, so the guard only trusts signed commits. The org's "Require signed commits" ruleset enforces signing on branches in our repos, but not on forks. +- It finds every PR in the push that triggered the run (`before...after`). A merge queue can merge several PRs in one push. +- It blocks approval by each PR's author and by whoever merged it. With GitHub's merge queue, `merged_by` is the person who added the PR to the queue. With Trunk, it's `trunk-io[bot]`, so only the author is checked. +- On events other than `push`, it does nothing, because GitHub's own check already excludes whoever triggered the run. +- It fails closed if the run has no approval for the environment, if a commit in the push isn't part of a merged PR, or if the push range can't be listed in full. - The approvals API doesn't say which run attempt an approval belongs to, so every approval in the run counts. After a block, a different approver must approve a **new run**. A re-run can still see the blocked approval. +- Like the setting it replaces, it only covers the PRs in the triggering push. If a release also publishes earlier unreleased changes, their authors aren't excluded. Run the tests with `node --test .github/actions/release-approval-guard/guard.test.mjs`. diff --git a/.github/actions/release-approval-guard/action.yaml b/.github/actions/release-approval-guard/action.yaml index 6c42199..e624f34 100644 --- a/.github/actions/release-approval-guard/action.yaml +++ b/.github/actions/release-approval-guard/action.yaml @@ -1,14 +1,10 @@ name: 'Release approval guard' -description: 'Fail a release when an approver of its environment authored or committed to a PR in the release' +description: 'Fail a release when an approver of its environment authored or merged a PR in the push that triggered it' inputs: environment: description: 'Name of the environment whose approval gates the release, e.g. "NPM Release"' required: true - changeset-dirs: - description: 'Newline-separated directories, relative to the checkout, that hold the pending changeset .md files' - required: false - default: '.changeset' github-token: description: 'Token with `actions: read`, `contents: read` and `pull-requests: read` on the repository' required: false @@ -17,11 +13,10 @@ inputs: runs: using: 'composite' steps: - - name: Check release approvers against release contributors + - name: Check release approvers against the PRs in the push shell: bash env: GH_TOKEN: ${{ inputs.github-token }} ENVIRONMENT: ${{ inputs.environment }} - CHANGESET_DIRS: ${{ inputs.changeset-dirs }} ACTION_PATH: ${{ github.action_path }} run: node "$ACTION_PATH/guard.mjs" diff --git a/.github/actions/release-approval-guard/guard.mjs b/.github/actions/release-approval-guard/guard.mjs index ba77c60..edd6232 100644 --- a/.github/actions/release-approval-guard/guard.mjs +++ b/.github/actions/release-approval-guard/guard.mjs @@ -1,56 +1,27 @@ #!/usr/bin/env node -import { execFileSync } from 'node:child_process'; -import { appendFileSync, existsSync, readdirSync } from 'node:fs'; -import { join } from 'node:path'; +import { appendFileSync, readFileSync } from 'node:fs'; import { fileURLToPath } from 'node:url'; -// GitHub's committer for web UI commits and squash merges. -const IGNORED_LOGINS = new Set(['web-flow']); -const PR_COMMITS_LIMIT = 250; - export class GuardError extends Error {} -export function listChangesets(dirs, cwd = process.cwd()) { - const files = []; - for (const dir of dirs) { - if (!existsSync(join(cwd, dir))) continue; - for (const name of readdirSync(join(cwd, dir))) { - if (name.endsWith('.md') && name.toLowerCase() !== 'readme.md') { - files.push(`${dir.replace(/\/+$/, '')}/${name}`); - } - } - } - return files.sort(); -} - -export function createApi({ token, apiUrl = 'https://api.github.com', fetchImpl = fetch }) { - async function request(url) { - const res = await fetchImpl(url, { - headers: { - Accept: 'application/vnd.github+json', - Authorization: `Bearer ${token}`, - 'X-GitHub-Api-Version': '2022-11-28', - }, - }); - if (!res.ok) { - throw new GuardError(`GitHub API ${res.status} for ${url}: ${await res.text()}`); - } - return res; - } - +export function createApi({ token, apiUrl = 'https://api.github.com', fetchImpl = fetch, retryDelayMs = 2000 }) { return { async get(path) { - return (await request(`${apiUrl}${path}`)).json(); - }, - async list(path) { - const items = []; - let url = `${apiUrl}${path}`; - while (url) { - const res = await request(url); - items.push(...(await res.json())); - url = res.headers.get('link')?.match(/<([^>]+)>;\s*rel="next"/)?.[1]; + for (let attempt = 1; ; attempt++) { + const res = await fetchImpl(`${apiUrl}${path}`, { + headers: { + Accept: 'application/vnd.github+json', + Authorization: `Bearer ${token}`, + 'X-GitHub-Api-Version': '2022-11-28', + }, + }); + if (res.ok) return res.json(); + if (res.status >= 500 && attempt < 3) { + await new Promise((resolve) => setTimeout(resolve, retryDelayMs * attempt)); + continue; + } + throw new GuardError(`GitHub API ${res.status} for ${path}: ${await res.text()}`); } - return items; }, }; } @@ -64,61 +35,26 @@ export async function approversFor(api, repo, runId, environment) { return [...new Set(logins)].sort(); } -export async function contributorsFor(api, repo, ref, files) { - const contributors = new Map(); - const add = (login, reason) => { - if (!login || IGNORED_LOGINS.has(login)) return; - const key = login.toLowerCase(); - if (!contributors.has(key)) contributors.set(key, { login, reasons: new Set() }); - contributors.get(key).reasons.add(reason); - }; - - const seenPrs = new Set(); - for (const file of files) { - const commits = await api.list( - `/repos/${repo}/commits?sha=${encodeURIComponent(ref)}&path=${encodeURIComponent(file)}&per_page=100`, - ); - if (commits.length === 0) { - throw new GuardError(`${file} has no commits at ${ref}. Is it committed?`); - } - for (const commit of commits) { - const prs = (await api.get(`/repos/${repo}/commits/${commit.sha}/pulls`)).filter( - (pr) => pr.merged_at, - ); - if (prs.length === 0) { - throw new GuardError( - `Commit ${commit.sha} changed ${file} but does not belong to a merged PR.`, - ); - } - for (const pr of prs) { - if (seenPrs.has(pr.number)) continue; - seenPrs.add(pr.number); - add(pr.user?.login, `author of #${pr.number}`); - const prCommits = await api.list(`/repos/${repo}/pulls/${pr.number}/commits?per_page=100`); - if (prCommits.length >= PR_COMMITS_LIMIT) { - throw new GuardError( - `#${pr.number} has ${PR_COMMITS_LIMIT} or more commits, the most GitHub lists, so its contributors cannot all be checked.`, - ); - } - for (const c of prCommits) { - if (!c.commit?.verification?.verified) { - throw new GuardError( - `Commit ${c.sha} in #${pr.number} has no verified signature, so its committer cannot be trusted.`, - ); - } - add(c.author?.login, `commit in #${pr.number}`); - add(c.committer?.login, `commit in #${pr.number}`); - } - } +export async function pushedPrs(api, repo, before, after) { + if (!before || /^0+$/.test(before)) { + throw new GuardError(`The push has no previous commit (before=${before}), so its PRs cannot be found.`); + } + const compare = await api.get(`/repos/${repo}/compare/${before}...${after}`); + if (compare.commits.length === 0 || compare.commits.length < compare.total_commits) { + throw new GuardError(`Cannot list all ${compare.total_commits} commits between ${before} and ${after}.`); + } + const numbers = new Set(); + for (const commit of compare.commits) { + const merged = (await api.get(`/repos/${repo}/commits/${commit.sha}/pulls`)).filter((pr) => pr.merged_at); + if (merged.length === 0) { + throw new GuardError(`Commit ${commit.sha} in the push does not belong to a merged PR.`); } + for (const pr of merged) numbers.add(pr.number); } - return contributors; + return Promise.all([...numbers].sort((a, b) => a - b).map((n) => api.get(`/repos/${repo}/pulls/${n}`))); } -export async function checkReleaseApproval({ api, repo, runId, environment, ref, files }) { - if (files.length === 0) { - throw new GuardError('No changesets found. Check the changeset-dirs input.'); - } +export async function checkReleaseApproval({ api, repo, runId, environment, before, after }) { const approvers = await approversFor(api, repo, runId, environment); if (approvers.length === 0) { throw new GuardError( @@ -126,59 +62,66 @@ export async function checkReleaseApproval({ api, repo, runId, environment, ref, 'Check the environment input and that the environment has required reviewers.', ); } - const contributors = await contributorsFor(api, repo, ref, files); - const violations = approvers - .filter((login) => contributors.has(login.toLowerCase())) - .map((login) => ({ login, reasons: [...contributors.get(login.toLowerCase()).reasons] })); - return { - approvers, - contributors: [...contributors.values()].map((c) => c.login).sort(), - violations, + const prs = await pushedPrs(api, repo, before, after); + const blocked = new Map(); + const block = (login, reason) => { + if (!login) return; + const key = login.toLowerCase(); + if (!blocked.has(key)) blocked.set(key, []); + blocked.get(key).push(reason); }; + for (const pr of prs) { + block(pr.user?.login, `author of #${pr.number}`); + block(pr.merged_by?.login, `merged #${pr.number}`); + } + const violations = approvers + .filter((login) => blocked.has(login.toLowerCase())) + .map((login) => ({ login, reasons: blocked.get(login.toLowerCase()) })); + return { approvers, prs: prs.map((pr) => pr.number), violations }; } async function main() { const env = process.env; + if (env.GITHUB_EVENT_NAME !== 'push') { + console.log( + `Not a push (${env.GITHUB_EVENT_NAME}), so GitHub's "Prevent self-review" already excludes whoever triggered the run.`, + ); + return; + } const environment = env.ENVIRONMENT?.trim(); if (!environment) throw new GuardError('The environment input is required.'); - const dirs = (env.CHANGESET_DIRS || '.changeset') - .split('\n') - .map((d) => d.trim()) - .filter(Boolean); - const ref = execFileSync('git', ['rev-parse', 'HEAD'], { encoding: 'utf8' }).trim(); - const files = listChangesets(dirs); + const event = JSON.parse(readFileSync(env.GITHUB_EVENT_PATH, 'utf8')); const result = await checkReleaseApproval({ api: createApi({ token: env.GH_TOKEN, apiUrl: env.GITHUB_API_URL }), repo: env.GITHUB_REPOSITORY, runId: env.GITHUB_RUN_ID, environment, - ref, - files, + before: event.before, + after: event.after, }); - console.log(`Changesets at ${ref}:\n ${files.join('\n ')}`); + console.log(`PRs in the push: ${result.prs.map((n) => `#${n}`).join(', ')}`); console.log(`Approvers of "${environment}": ${result.approvers.join(', ')}`); - console.log(`Contributors to released PRs: ${result.contributors.join(', ')}`); if (result.violations.length > 0) { const lines = result.violations.map((v) => `${v.login} (${v.reasons.join(', ')})`); console.log( - `::error title=Release approved by a contributor::${lines.join('; ')} approved a release ` + - 'that contains their own changes. Another approver must approve a new run of this ' + + `::error title=Release approved by its author::${lines.join('; ')} approved a release ` + + 'triggered by their own PR. Another approver must approve a new run of this ' + 'workflow. A re-run of this run can still see the earlier approval and fail again.', ); if (env.GITHUB_STEP_SUMMARY) { appendFileSync( env.GITHUB_STEP_SUMMARY, - `### Release blocked\n\nApproved by a contributor to this release:\n\n${lines + `### Release blocked\n\nApproved by the author or merger of a PR in this push:\n\n${lines .map((l) => `- ${l}`) .join('\n')}\n`, ); } process.exit(1); } - console.log('✓ No approver contributed to the released changes.'); + console.log('✓ No approver authored or merged a PR in this push.'); } if (process.argv[1] === fileURLToPath(import.meta.url)) { diff --git a/.github/actions/release-approval-guard/guard.test.mjs b/.github/actions/release-approval-guard/guard.test.mjs index 5b0e2ef..aad616a 100644 --- a/.github/actions/release-approval-guard/guard.test.mjs +++ b/.github/actions/release-approval-guard/guard.test.mjs @@ -1,29 +1,26 @@ import assert from 'node:assert/strict'; -import { mkdirSync, mkdtempSync, rmSync, writeFileSync } from 'node:fs'; -import { tmpdir } from 'node:os'; -import { join } from 'node:path'; +import { execFileSync } from 'node:child_process'; import test from 'node:test'; +import { fileURLToPath } from 'node:url'; -import { checkReleaseApproval, createApi, GuardError, listChangesets } from './guard.mjs'; +import { checkReleaseApproval, createApi, GuardError } from './guard.mjs'; const repo = 'PostHog/posthog-js'; -const ref = 'head-sha'; const API = 'https://api.github.test'; +const before = 'b'.repeat(40); +const after = 'a'.repeat(40); -function fakeApi(routes) { +function fakeApi(routes, failures = {}) { const fetchImpl = async (url) => { const path = url.slice(API.length); - const [route, page] = path.split('&page='); - if (!(route in routes)) return new Response(`no route ${path}`, { status: 404 }); - const body = routes[route]; - if (Array.isArray(body) && Array.isArray(body[0])) { - const n = Number(page || 1); - const headers = n < body.length ? { link: `<${API}${route}&page=${n + 1}>; rel="next"` } : {}; - return Response.json(body[n - 1], { headers }); + if (failures[path] > 0) { + failures[path]--; + return new Response('unavailable', { status: 502 }); } - return Response.json(body); + if (!(path in routes)) return new Response(`no route ${path}`, { status: 404 }); + return Response.json(routes[path]); }; - return createApi({ token: 't', apiUrl: API, fetchImpl }); + return createApi({ token: 't', apiUrl: API, fetchImpl, retryDelayMs: 0 }); } const approval = (login, environment = 'NPM Release', state = 'approved') => ({ @@ -31,134 +28,120 @@ const approval = (login, environment = 'NPM Release', state = 'approved') => ({ user: { login }, environments: [{ name: environment }], }); -const commitsFor = (file) => `/repos/${repo}/commits?sha=${ref}&path=${encodeURIComponent(file)}&per_page=100`; -const pullsFor = (sha) => `/repos/${repo}/commits/${sha}/pulls`; -const prCommits = (n) => `/repos/${repo}/pulls/${n}/commits?per_page=100`; -const pr = (number, login) => ({ number, merged_at: '2026-10-01T00:00:00Z', user: { login } }); -const commit = (author, committer = 'web-flow', verified = true) => ({ - sha: 'abcdef0123', - author: author && { login: author }, - committer: committer && { login: committer }, - commit: { verification: { verified } }, -}); -function singlePr({ author = 'alice', approvals = [approval('bob')], commits = [commit(author)] } = {}) { - return { +// Each entry is [commit sha, PR number, author, merged_by]. +function push(approvals, prs) { + const routes = { [`/repos/${repo}/actions/runs/7/approvals`]: approvals, - [commitsFor('.changeset/a.md')]: [{ sha: 'c1' }], - [pullsFor('c1')]: [pr(1, author)], - [prCommits(1)]: commits, + [`/repos/${repo}/compare/${before}...${after}`]: { + total_commits: prs.length, + commits: prs.map(([sha]) => ({ sha })), + }, }; + for (const [sha, number, author, mergedBy] of prs) { + routes[`/repos/${repo}/commits/${sha}/pulls`] = [{ number, merged_at: '2026-10-01T00:00:00Z' }]; + routes[`/repos/${repo}/pulls/${number}`] = { + number, + user: { login: author }, + merged_by: mergedBy && { login: mergedBy }, + }; + } + return routes; } -const check = (routes, files = ['.changeset/a.md'], environment = 'NPM Release') => - checkReleaseApproval({ api: fakeApi(routes), repo, runId: 7, environment, ref, files }); +const check = (routes, environment = 'NPM Release') => + checkReleaseApproval({ api: fakeApi(routes), repo, runId: 7, environment, before, after }); -test('passes when no approver contributed to the release', async () => { - const result = await check(singlePr()); +test('passes when no approver authored or merged a PR in the push', async () => { + const result = await check(push([approval('bob')], [['c1', 1, 'alice', 'alice']])); assert.deepEqual(result.violations, []); - assert.deepEqual(result.approvers, ['bob']); - assert.deepEqual(result.contributors, ['alice']); -}); - -test('blocks the PR author approving their own release', async () => { - const result = await check(singlePr({ approvals: [approval('alice')] })); - assert.deepEqual(result.violations, [{ login: 'alice', reasons: ['author of #1', 'commit in #1'] }]); + assert.deepEqual(result.prs, [1]); }); -test('blocks someone who pushed commits to another person\'s PR', async () => { - const result = await check(singlePr({ commits: [commit('alice'), commit('bob', 'bob')] })); - assert.deepEqual(result.violations, [{ login: 'bob', reasons: ['commit in #1'] }]); +test('blocks the PR author', async () => { + const result = await check(push([approval('alice')], [['c1', 1, 'alice', 'github-merge-queue[bot]']])); + assert.deepEqual(result.violations, [{ login: 'alice', reasons: ['author of #1'] }]); }); -test('matches logins case-insensitively', async () => { - const result = await check(singlePr({ approvals: [approval('Alice')] })); - assert.equal(result.violations[0].login, 'Alice'); +test('blocks whoever merged the PR', async () => { + const result = await check(push([approval('bob')], [['c1', 1, 'external', 'bob']])); + assert.deepEqual(result.violations, [{ login: 'bob', reasons: ['merged #1'] }]); }); -test('blocks when any approval in the run came from a contributor', async () => { - const result = await check(singlePr({ approvals: [approval('bob'), approval('alice')] })); - assert.deepEqual(result.violations.map((v) => v.login), ['alice']); -}); - -test('checks every PR in a batched release, including PRs that edited a changeset', async () => { - const routes = { - [`/repos/${repo}/actions/runs/7/approvals`]: [approval('carol')], - [commitsFor('.changeset/a.md')]: [{ sha: 'c1' }], - [commitsFor('.changeset/b.md')]: [{ sha: 'c3' }, { sha: 'c2' }], - [pullsFor('c1')]: [pr(1, 'alice')], - [pullsFor('c2')]: [pr(2, 'bob')], - [pullsFor('c3')]: [pr(3, 'carol')], - [prCommits(1)]: [commit('alice')], - [prCommits(2)]: [commit('bob')], - [prCommits(3)]: [commit('carol')], - }; - const result = await check(routes, ['.changeset/a.md', '.changeset/b.md']); - assert.deepEqual(result.contributors, ['alice', 'bob', 'carol']); +test('checks every PR in a batched merge queue push', async () => { + const prs = [ + ['c1', 1, 'alice', 'alice'], + ['c2', 2, 'carol', 'carol'], + ]; + const result = await check(push([approval('carol')], prs)); + assert.deepEqual(result.prs, [1, 2]); assert.deepEqual(result.violations.map((v) => v.login), ['carol']); }); -test('fails closed when a PR has a commit without a verified signature', async () => { - const routes = singlePr({ commits: [commit('alice'), commit('bob', 'bob', false)] }); - await assert.rejects(check(routes), /abcdef0123 in #1 has no verified signature/); -}); - -test('fails closed when a PR reaches the 250-commit listing limit', async () => { - const routes = singlePr(); - routes[prCommits(1)] = [Array(100).fill(commit('alice')), Array(100).fill(commit('alice')), Array(50).fill(commit('alice'))]; - await assert.rejects(check(routes), /#1 has 250 or more commits/); +test('matches logins case-insensitively', async () => { + const result = await check(push([approval('Alice')], [['c1', 1, 'alice', 'alice']])); + assert.equal(result.violations[0].login, 'Alice'); }); -test('follows pagination of PR commits', async () => { - const routes = singlePr(); - routes[prCommits(1)] = [[commit('alice')], [commit('bob')]]; - const result = await check(routes); - assert.deepEqual(result.violations.map((v) => v.login), ['bob']); +test('blocks when any approval in the run came from an author', async () => { + const result = await check(push([approval('bob'), approval('alice')], [['c1', 1, 'alice', 'alice']])); + assert.deepEqual(result.violations.map((v) => v.login), ['alice']); }); test('ignores approvals that are rejected or for other environments', async () => { const approvals = [approval('alice', 'S3 Upload'), approval('alice', 'NPM Release', 'rejected'), approval('bob')]; - const result = await check(singlePr({ approvals })); + const result = await check(push(approvals, [['c1', 1, 'alice', 'alice']])); assert.deepEqual(result.approvers, ['bob']); assert.deepEqual(result.violations, []); }); test('fails closed when the run has no approval for the environment', async () => { - await assert.rejects(check(singlePr({ approvals: [approval('bob', 'Other')] })), GuardError); + await assert.rejects(check(push([approval('bob', 'Other')], [['c1', 1, 'alice', 'alice']])), GuardError); }); -test('fails closed when a changeset commit is not from a merged PR', async () => { - const routes = singlePr(); - routes[pullsFor('c1')] = [{ ...pr(1, 'alice'), merged_at: null }]; - await assert.rejects(check(routes), /does not belong to a merged PR/); +test('fails closed when a commit in the push is not from a merged PR', async () => { + const routes = push([approval('bob')], [['c1', 1, 'alice', 'alice']]); + routes[`/repos/${repo}/commits/c1/pulls`] = [{ number: 1, merged_at: null }]; + await assert.rejects(check(routes), /c1 in the push does not belong to a merged PR/); }); -test('fails closed when a changeset has no commits', async () => { - const routes = singlePr(); - routes[commitsFor('.changeset/a.md')] = []; - await assert.rejects(check(routes), /has no commits/); +test('fails closed when the comparison does not list every commit', async () => { + const routes = push([approval('bob')], [['c1', 1, 'alice', 'alice']]); + routes[`/repos/${repo}/compare/${before}...${after}`].total_commits = 300; + await assert.rejects(check(routes), /Cannot list all 300 commits/); }); -test('fails closed when there are no changesets', async () => { - await assert.rejects(check(singlePr(), []), /No changesets found/); +test('fails closed when the push has no previous commit', async () => { + const routes = push([approval('bob')], [['c1', 1, 'alice', 'alice']]); + await assert.rejects( + checkReleaseApproval({ api: fakeApi(routes), repo, runId: 7, environment: 'NPM Release', before: '0'.repeat(40), after }), + /has no previous commit/, + ); }); test('surfaces GitHub API errors', async () => { - const routes = singlePr(); - delete routes[prCommits(1)]; + const routes = push([approval('bob')], [['c1', 1, 'alice', 'alice']]); + delete routes[`/repos/${repo}/pulls/1`]; await assert.rejects(check(routes), /GitHub API 404/); }); -test('lists pending changesets across directories', (t) => { - const cwd = mkdtempSync(join(tmpdir(), 'release-approval-guard-')); - t.after(() => rmSync(cwd, { recursive: true, force: true })); - for (const file of ['.changeset/b.md', '.changeset/a.md', '.changeset/README.md', '.changeset/config.json', 'cli/.sampo/changesets/c.md']) { - mkdirSync(join(cwd, file, '..'), { recursive: true }); - writeFileSync(join(cwd, file), ''); - } - assert.deepEqual(listChangesets(['.changeset', 'cli/.sampo/changesets/', 'missing'], cwd), [ - '.changeset/a.md', - '.changeset/b.md', - 'cli/.sampo/changesets/c.md', - ]); +test('retries GitHub server errors', async () => { + const routes = push([approval('bob')], [['c1', 1, 'alice', 'alice']]); + const compare = `/repos/${repo}/compare/${before}...${after}`; + const api = fakeApi(routes, { [compare]: 2 }); + const result = await checkReleaseApproval({ api, repo, runId: 7, environment: 'NPM Release', before, after }); + assert.deepEqual(result.prs, [1]); + await assert.rejects( + checkReleaseApproval({ api: fakeApi(routes, { [compare]: 3 }), repo, runId: 7, environment: 'NPM Release', before, after }), + /GitHub API 502/, + ); +}); + +test('leaves runs that are not pushes to GitHub\'s own check', () => { + const script = fileURLToPath(new URL('./guard.mjs', import.meta.url)); + const out = execFileSync('node', [script], { + env: { ...process.env, GITHUB_EVENT_NAME: 'workflow_dispatch' }, + encoding: 'utf8', + }); + assert.match(out, /Not a push \(workflow_dispatch\)/); });