From 71adbfe4bf1dd9be4ab5debcb4a5b70789306d26 Mon Sep 17 00:00:00 2001 From: Ioannis J Date: Thu, 22 Oct 2026 13:12:45 +0300 Subject: [PATCH 1/2] feat(ci): name excluded approvers in the release approval Slack message --- .../actions/release-approval-guard/README.md | 39 +++- .../release-approval-guard/action.yaml | 32 +++- .../actions/release-approval-guard/guard.mjs | 161 ++++++++++++----- .../release-approval-guard/guard.test.mjs | 141 +++++++++++++-- .github/workflows/notify-approval-needed.yml | 170 +++++++++++------- 5 files changed, 418 insertions(+), 125 deletions(-) diff --git a/.github/actions/release-approval-guard/README.md b/.github/actions/release-approval-guard/README.md index 35ccf50..0af4413 100644 --- a/.github/actions/release-approval-guard/README.md +++ b/.github/actions/release-approval-guard/README.md @@ -1,6 +1,6 @@ # Release approval guard -Fails a release when someone who approved its environment authored or merged a PR in the push that triggered the run. +Fails a release when someone who approved its environment authored or queued a PR in the push that triggered the run. In `report` mode it only names who is excluded, so the approval request can say so. 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. @@ -14,7 +14,7 @@ jobs: environment: 'NPM Release' permissions: contents: read - actions: read # approvals of this run + actions: read # approvals of this run, `enforce` mode only pull-requests: read # PRs in the push steps: - uses: PostHog/.github/.github/actions/release-approval-guard@ @@ -24,13 +24,44 @@ jobs: It needs no checkout. It needs Node, which GitHub-hosted runners include. +## Inputs + +| Input | Default | Purpose | +| -------------- | ---------------- | -------------------------------------------------------------------------------------------- | +| `environment` | | Environment whose approval gates the release. Required in `enforce` mode. | +| `mode` | `enforce` | `enforce` fails the job on a blocked approval. `report` never fails and only writes outputs. | +| `github-token` | `github.token` | Token for the GitHub API. | + +## Report mode + +Use `mode: report` before the approval exists, for example in the job that posts the approval request to Slack: + +```yaml +- id: guard + uses: PostHog/.github/.github/actions/release-approval-guard@ + with: + mode: report +``` + +It lists the PRs in the push and who can't approve them. It never calls the approvals API, so it needs only `contents: read` and `pull-requests: read`, not `actions: read`. On any error it logs a warning and writes empty outputs. Both modes write these outputs, empty when the event isn't a `push`: + +| Output | Example | +| ---------------- | --------------------------------------------------------------- | +| `blocked_logins` | `alice,bob` | +| `blocked_mrkdwn` | ` (queued #5273), (author of #5270)` | +| `violations_mrkdwn` | ` (queued #5273)` (approvers who violated the rule; empty in `report` mode and when none did) | +| `prs_mrkdwn` | ` fix: flush queue` (one PR per line) | +| `prs` | `5270,5273` | + +Titles are escaped for Slack (`&`, `<`, `>`). `prs_mrkdwn` is capped to fit a Slack field. Bots such as the merge queue are left out of the blocked list. + ## Behavior - 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. +- It blocks approval by each PR's author and by whoever queued 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. +- After a block, re-run the failed jobs. That requests a new approval, and a different approver must give it. The approvals endpoint then lists only the new approval, so the guard passes on the next attempt. - 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 e624f34..c04f728 100644 --- a/.github/actions/release-approval-guard/action.yaml +++ b/.github/actions/release-approval-guard/action.yaml @@ -1,22 +1,46 @@ name: 'Release approval guard' -description: 'Fail a release when an approver of its environment authored or merged a PR in the push that triggered it' +description: 'Fail a release when an approver of its environment authored or queued a PR in the push that triggered it, or report who is excluded' inputs: environment: - description: 'Name of the environment whose approval gates the release, e.g. "NPM Release"' - required: true + description: 'Name of the environment whose approval gates the release, e.g. "NPM Release". Required in `enforce` mode.' + required: false + default: '' + mode: + description: '`enforce` fails the job when an approver authored or queued a PR in the push. `report` only lists the PRs and who is excluded, never fails, and needs no approval to exist yet.' + required: false + default: 'enforce' github-token: - description: 'Token with `actions: read`, `contents: read` and `pull-requests: read` on the repository' + description: 'Token with `contents: read` and `pull-requests: read` on the repository, plus `actions: read` in `enforce` mode' required: false default: ${{ github.token }} +outputs: + blocked_logins: + description: 'Comma-separated logins excluded from approving: authors and enqueuers of the PRs in the push, bots omitted' + value: ${{ steps.guard.outputs.blocked_logins }} + blocked_mrkdwn: + description: 'Slack mrkdwn list of the excluded logins with the reason, e.g. ` (queued #5273)`' + value: ${{ steps.guard.outputs.blocked_mrkdwn }} + violations_mrkdwn: + description: 'Slack mrkdwn list of the approvers who violated the rule, same format as `blocked_mrkdwn`. Empty in `report` mode and when there are none.' + value: ${{ steps.guard.outputs.violations_mrkdwn }} + prs_mrkdwn: + description: 'Slack mrkdwn list of the PRs in the push, one ` title` per line' + value: ${{ steps.guard.outputs.prs_mrkdwn }} + prs: + description: 'Comma-separated numbers of the PRs in the push' + value: ${{ steps.guard.outputs.prs }} + runs: using: 'composite' steps: - name: Check release approvers against the PRs in the push + id: guard shell: bash env: GH_TOKEN: ${{ inputs.github-token }} ENVIRONMENT: ${{ inputs.environment }} + MODE: ${{ inputs.mode }} 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 edd6232..e825cd3 100644 --- a/.github/actions/release-approval-guard/guard.mjs +++ b/.github/actions/release-approval-guard/guard.mjs @@ -1,5 +1,6 @@ #!/usr/bin/env node import { appendFileSync, readFileSync } from 'node:fs'; +import { randomUUID } from 'node:crypto'; import { fileURLToPath } from 'node:url'; export class GuardError extends Error {} @@ -26,7 +27,6 @@ export function createApi({ token, apiUrl = 'https://api.github.com', fetchImpl }; } -// 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 @@ -44,84 +44,159 @@ export async function pushedPrs(api, repo, before, after) { 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); + const mergedPerCommit = await Promise.all( + compare.commits.map(async (commit) => ({ + sha: commit.sha, + merged: (await api.get(`/repos/${repo}/commits/${commit.sha}/pulls`)).filter((pr) => pr.merged_at), + })), + ); + for (const { sha, merged } of mergedPerCommit) { if (merged.length === 0) { - throw new GuardError(`Commit ${commit.sha} in the push does not belong to a merged PR.`); + throw new GuardError(`Commit ${sha} in the push does not belong to a merged PR.`); } for (const pr of merged) numbers.add(pr.number); } 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, before, after }) { - 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 prs = await pushedPrs(api, repo, before, after); +export function blockedApprovers(prs) { const blocked = new Map(); const block = (login, reason) => { - if (!login) return; + if (!login || login.endsWith('[bot]')) return; const key = login.toLowerCase(); - if (!blocked.has(key)) blocked.set(key, []); - blocked.get(key).push(reason); + if (!blocked.has(key)) blocked.set(key, { login, reasons: [] }); + blocked.get(key).reasons.push(reason); }; for (const pr of prs) { block(pr.user?.login, `author of #${pr.number}`); - block(pr.merged_by?.login, `merged #${pr.number}`); + block(pr.merged_by?.login, `queued #${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 }; + return [...blocked.values()].sort((a, b) => a.login.toLowerCase().localeCompare(b.login.toLowerCase())); } -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.`, +const escapeMrkdwn = (text) => text.replace(/&/g, '&').replace(//g, '>'); + +export function blockedMrkdwn(blocked) { + return blocked + .map(({ login, reasons }) => ` (${reasons.join(', ')})`) + .join(', '); +} + +// Slack caps a section field at 2000 characters. +export function prsMrkdwn(prs, maxLength = 1800) { + const lines = []; + let length = 0; + for (const pr of prs) { + const line = `<${pr.html_url}|#${pr.number}> ${escapeMrkdwn(pr.title ?? '')}`.trimEnd(); + if (length + line.length > maxLength) { + lines.push(`…and ${prs.length - lines.length} more`); + break; + } + lines.push(line); + length += line.length + 1; + } + return lines.join('\n'); +} + +export function guardOutputs(prs, blocked, violations = []) { + return { + blocked_logins: blocked.map((b) => b.login).join(','), + blocked_mrkdwn: blockedMrkdwn(blocked), + violations_mrkdwn: blockedMrkdwn(violations), + prs_mrkdwn: prsMrkdwn(prs), + prs: prs.map((pr) => pr.number).join(','), + }; +} + +const EMPTY_OUTPUTS = guardOutputs([], []); + +export function writeOutputs(file, outputs) { + if (!file) return; + for (const [name, value] of Object.entries(outputs)) { + const delimiter = `ghadelimiter_${randomUUID()}`; + appendFileSync(file, `${name}<<${delimiter}\n${value}\n${delimiter}\n`); + } +} + +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( + `No approval for the "${environment}" environment in this run. ` + + 'Check the environment input and that the environment has required reviewers.', ); - return; } + const pullRequests = await pushedPrs(api, repo, before, after); + const blocked = blockedApprovers(pullRequests); + const approverSet = new Set(approvers.map((login) => login.toLowerCase())); + const violations = blocked.filter((b) => approverSet.has(b.login.toLowerCase())); + return { approvers, pullRequests, blocked, violations }; +} + +function pushContext(env) { + const { before, after } = JSON.parse(readFileSync(env.GITHUB_EVENT_PATH, 'utf8')); + return { api: createApi({ token: env.GH_TOKEN, apiUrl: env.GITHUB_API_URL }), repo: env.GITHUB_REPOSITORY, before, after }; +} + +async function runReport(env) { + try { + const { api, repo, before, after } = pushContext(env); + const prs = await pushedPrs(api, repo, before, after); + const outputs = guardOutputs(prs, blockedApprovers(prs)); + writeOutputs(env.GITHUB_OUTPUT, outputs); + console.log(`PRs in the push: ${outputs.prs}`); + console.log(`Excluded from approving: ${outputs.blocked_logins || 'none'}`); + } catch (e) { + console.log(`::warning title=Release approval guard::${e.message}`); + writeOutputs(env.GITHUB_OUTPUT, EMPTY_OUTPUTS); + } +} + +async function runEnforce(env) { const environment = env.ENVIRONMENT?.trim(); if (!environment) throw new GuardError('The environment input is required.'); - 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, - before: event.before, - after: event.after, - }); + const { api, repo, before, after } = pushContext(env); - console.log(`PRs in the push: ${result.prs.map((n) => `#${n}`).join(', ')}`); + const result = await checkReleaseApproval({ api, repo, runId: env.GITHUB_RUN_ID, environment, before, after }); + writeOutputs(env.GITHUB_OUTPUT, guardOutputs(result.pullRequests, result.blocked, result.violations)); + + console.log(`PRs in the push: ${result.pullRequests.map((p) => `#${p.number}`).join(', ')}`); console.log(`Approvers of "${environment}": ${result.approvers.join(', ')}`); if (result.violations.length > 0) { const lines = result.violations.map((v) => `${v.login} (${v.reasons.join(', ')})`); console.log( `::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.', + 'triggered by their own PR. Re-run the failed jobs to request a new approval, ' + + 'and have a different approver approve it.', ); if (env.GITHUB_STEP_SUMMARY) { appendFileSync( env.GITHUB_STEP_SUMMARY, - `### Release blocked\n\nApproved by the author or merger of a PR in this push:\n\n${lines + `### Release blocked\n\nApproved by the author or enqueuer of a PR in this push:\n\n${lines .map((l) => `- ${l}`) - .join('\n')}\n`, + .join('\n')}\n\nRe-run the failed jobs to request a new approval. A different approver must approve it.\n`, ); } process.exit(1); } - console.log('βœ“ No approver authored or merged a PR in this push.'); + console.log('βœ“ No approver authored or queued a PR in this push.'); +} + +async function main() { + const env = process.env; + const mode = env.MODE?.trim() || 'enforce'; + if (mode !== 'enforce' && mode !== 'report') { + throw new GuardError(`Unknown mode "${mode}". Use "enforce" or "report".`); + } + 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.`, + ); + writeOutputs(env.GITHUB_OUTPUT, EMPTY_OUTPUTS); + return; + } + await (mode === 'report' ? runReport(env) : runEnforce(env)); } 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 aad616a..a0d2cae 100644 --- a/.github/actions/release-approval-guard/guard.test.mjs +++ b/.github/actions/release-approval-guard/guard.test.mjs @@ -1,12 +1,25 @@ import assert from 'node:assert/strict'; import { execFileSync } from 'node:child_process'; import test from 'node:test'; +import { mkdtempSync, readFileSync } from 'node:fs'; +import { tmpdir } from 'node:os'; +import { join } from 'node:path'; import { fileURLToPath } from 'node:url'; -import { checkReleaseApproval, createApi, GuardError } from './guard.mjs'; +import { + blockedApprovers, + blockedMrkdwn, + checkReleaseApproval, + createApi, + GuardError, + guardOutputs, + prsMrkdwn, + writeOutputs, +} from './guard.mjs'; const repo = 'PostHog/posthog-js'; const API = 'https://api.github.test'; +const script = fileURLToPath(new URL('./guard.mjs', import.meta.url)); const before = 'b'.repeat(40); const after = 'a'.repeat(40); @@ -42,6 +55,8 @@ function push(approvals, prs) { routes[`/repos/${repo}/commits/${sha}/pulls`] = [{ number, merged_at: '2026-10-01T00:00:00Z' }]; routes[`/repos/${repo}/pulls/${number}`] = { number, + html_url: `https://github.com/${repo}/pull/${number}`, + title: `PR ${number}`, user: { login: author }, merged_by: mergedBy && { login: mergedBy }, }; @@ -55,7 +70,7 @@ const check = (routes, environment = 'NPM Release') => 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.prs, [1]); + assert.deepEqual(result.pullRequests.map((p) => p.number), [1]); }); test('blocks the PR author', async () => { @@ -63,9 +78,9 @@ test('blocks the PR author', async () => { assert.deepEqual(result.violations, [{ login: 'alice', reasons: ['author of #1'] }]); }); -test('blocks whoever merged the PR', async () => { +test('blocks whoever queued the PR', async () => { const result = await check(push([approval('bob')], [['c1', 1, 'external', 'bob']])); - assert.deepEqual(result.violations, [{ login: 'bob', reasons: ['merged #1'] }]); + assert.deepEqual(result.violations, [{ login: 'bob', reasons: ['queued #1'] }]); }); test('checks every PR in a batched merge queue push', async () => { @@ -74,13 +89,13 @@ test('checks every PR in a batched merge queue push', async () => { ['c2', 2, 'carol', 'carol'], ]; const result = await check(push([approval('carol')], prs)); - assert.deepEqual(result.prs, [1, 2]); + assert.deepEqual(result.pullRequests.map((p) => p.number), [1, 2]); assert.deepEqual(result.violations.map((v) => v.login), ['carol']); }); test('matches logins case-insensitively', async () => { const result = await check(push([approval('Alice')], [['c1', 1, 'alice', 'alice']])); - assert.equal(result.violations[0].login, 'Alice'); + assert.deepEqual(result.violations.map((v) => v.login), ['alice']); }); test('blocks when any approval in the run came from an author', async () => { @@ -130,18 +145,120 @@ test('retries GitHub server errors', async () => { 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]); + assert.deepEqual(result.pullRequests.map((p) => p.number), [1]); await assert.rejects( checkReleaseApproval({ api: fakeApi(routes, { [compare]: 3 }), repo, runId: 7, environment: 'NPM Release', before, after }), /GitHub API 502/, ); }); +const run = (env, options = {}) => + execFileSync('node', [script], { env: { ...process.env, ...env }, encoding: 'utf8', ...options }); + 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(run({ GITHUB_EVENT_NAME: 'workflow_dispatch' }), /Not a push \(workflow_dispatch\)/); +}); + +const pr = (number, author, mergedBy, title = `PR ${number}`) => ({ + number, + title, + html_url: `https://github.com/${repo}/pull/${number}`, + user: author && { login: author }, + merged_by: mergedBy && { login: mergedBy }, +}); + +test('blockedApprovers lists authors and enqueuers sorted by login', () => { + const blocked = blockedApprovers([pr(2, 'zed', 'bob'), pr(1, 'alice', 'bob')]); + assert.deepEqual(blocked, [ + { login: 'alice', reasons: ['author of #1'] }, + { login: 'bob', reasons: ['queued #2', 'queued #1'] }, + { login: 'zed', reasons: ['author of #2'] }, + ]); +}); + +test('blockedApprovers dedupes logins case-insensitively and keeps the first casing', () => { + const blocked = blockedApprovers([pr(1, 'Alice', 'alice')]); + assert.deepEqual(blocked, [{ login: 'Alice', reasons: ['author of #1', 'queued #1'] }]); +}); + +test('blockedApprovers skips bots and missing users', () => { + const blocked = blockedApprovers([pr(1, 'dependabot[bot]', 'github-merge-queue[bot]'), pr(2, null, null)]); + assert.deepEqual(blocked, []); +}); + +test('blockedMrkdwn links each login with its reasons', () => { + const blocked = [ + { login: 'alice', reasons: ['queued #5273'] }, + { login: 'bob', reasons: ['author of #1', 'queued #2'] }, + ]; + assert.equal( + blockedMrkdwn(blocked), + ' (queued #5273), (author of #1, queued #2)', + ); + assert.equal(blockedMrkdwn([]), ''); +}); + +test('prsMrkdwn escapes Slack control characters in titles', () => { + const out = prsMrkdwn([pr(1, 'a', 'b', 'fix & "quotes"'), pr(2, 'a', 'b')]); + assert.equal( + out, + ` fix <b> & "quotes"\n PR 2`, + ); +}); + +test('prsMrkdwn truncates to the length limit', () => { + const prs = [pr(1, 'a', 'b', 'x'.repeat(50)), pr(2, 'a', 'b', 'y'.repeat(50)), pr(3, 'a', 'b', 'z'.repeat(50))]; + const out = prsMrkdwn(prs, 130); + assert.equal(out.split('\n').length, 2); + assert.match(out, /…and 2 more$/); +}); + +test('guardOutputs summarizes the push', () => { + const prs = [pr(1, 'alice', 'bob'), pr(2, 'carol', 'github-merge-queue[bot]')]; + const outputs = guardOutputs(prs, blockedApprovers(prs)); + assert.equal(outputs.blocked_logins, 'alice,bob,carol'); + assert.equal(outputs.prs, '1,2'); + assert.match(outputs.prs_mrkdwn, /^<.*\|#1> PR 1\n<.*\|#2> PR 2$/); +}); + +test('writeOutputs writes multiline values as heredocs', () => { + const file = join(mkdtempSync(join(tmpdir(), 'guard-')), 'output'); + writeOutputs(file, { prs: '1,2', prs_mrkdwn: 'a\nb' }); + const text = readFileSync(file, 'utf8'); + assert.match(text, /^prs<<(ghadelimiter_[\w-]+)\n1,2\n\1\nprs_mrkdwn<<(ghadelimiter_[\w-]+)\na\nb\n\2\n$/); +}); + +test('checkReleaseApproval exposes the blocked list and PRs for the outputs', async () => { + const result = await check(push([approval('alice')], [['c1', 1, 'alice', 'github-merge-queue[bot]']])); + assert.deepEqual(result.blocked, [{ login: 'alice', reasons: ['author of #1'] }]); + assert.deepEqual(result.pullRequests.map((p) => p.number), [1]); +}); + +test('report mode warns and writes empty outputs on errors', () => { + const dir = mkdtempSync(join(tmpdir(), 'guard-')); + const output = join(dir, 'output'); + const out = run({ + MODE: 'report', + GITHUB_EVENT_NAME: 'push', + GITHUB_EVENT_PATH: join(dir, 'missing.json'), + GITHUB_OUTPUT: output, }); - assert.match(out, /Not a push \(workflow_dispatch\)/); + assert.match(out, /::warning title=Release approval guard::/); + assert.match(readFileSync(output, 'utf8'), /blocked_logins< { + const script = fileURLToPath(new URL('./guard.mjs', import.meta.url)); + assert.throws( + () => run({ MODE: 'bogus' }, { stdio: 'pipe' }), + (e) => /Unknown mode "bogus"/.test(e.stdout), + ); +}); + +test('guardOutputs lists only violating approvers in violations_mrkdwn', async () => { + const result = await check(push([approval('alice')], [['c1', 1, 'alice', 'bob']])); + const outputs = guardOutputs(result.pullRequests, result.blocked, result.violations); + assert.equal(outputs.violations_mrkdwn, ' (author of #1)'); + assert.match(outputs.blocked_mrkdwn, /bob/); + assert.equal(guardOutputs(result.pullRequests, result.blocked).violations_mrkdwn, ''); }); diff --git a/.github/workflows/notify-approval-needed.yml b/.github/workflows/notify-approval-needed.yml index 4ab3108..6f742aa 100644 --- a/.github/workflows/notify-approval-needed.yml +++ b/.github/workflows/notify-approval-needed.yml @@ -24,6 +24,11 @@ on: required: false type: string default: "creatordate" + guard_ref: + description: "Ref of PostHog/.github to fetch the release approval guard from. Point at a branch while iterating; callers may pin a SHA." + required: false + type: string + default: "main" secrets: slack_bot_token: required: true @@ -34,6 +39,8 @@ on: description: "Slack message timestamp for threading replies" value: ${{ jobs.notify.outputs.slack_ts }} +# When the caller grants `contents: read` and `pull-requests: read` to this job, the message also names who +# cannot approve (authors and enqueuers of the PRs in the push) and lists the PRs. Without them it still sends. jobs: notify: runs-on: ubuntu-latest @@ -109,6 +116,106 @@ jobs: fi echo "default_branch=$DEFAULT_BRANCH" >> "$GITHUB_OUTPUT" + - name: Checkout release approval guard + id: guard-source + if: github.event_name == 'push' + continue-on-error: true + uses: actions/checkout@de0fac2e4500dabe0009e67214ff5f5447ce83dd # v6.0.2 + with: + repository: PostHog/.github + ref: ${{ inputs.guard_ref }} + path: .github-shared + sparse-checkout: .github/actions/release-approval-guard + persist-credentials: false + + - name: List who cannot approve + id: guard + if: github.event_name == 'push' && steps.guard-source.outcome == 'success' + continue-on-error: true + uses: ./.github-shared/.github/actions/release-approval-guard + with: + mode: report + github-token: ${{ github.token }} + + - name: Build Slack payload + id: payload + shell: bash + env: + CHANNEL: ${{ inputs.slack_channel_id }} + USER_GROUP: ${{ inputs.slack_user_group_id }} + REPOSITORY: ${{ github.repository }} + RUN_ID: ${{ github.run_id }} + LATEST_TAG: ${{ steps.get-info.outputs.latest_tag }} + DEFAULT_BRANCH: ${{ steps.get-info.outputs.default_branch }} + BLOCKED_MRKDWN: ${{ steps.guard.outputs.blocked_mrkdwn }} + PRS_MRKDWN: ${{ steps.guard.outputs.prs_mrkdwn }} + run: | + set -euo pipefail + + payload=$(jq -nc ' + { + channel: $ENV.CHANNEL, + text: "πŸš€ Release Approval Needed for \($ENV.REPOSITORY)", + blocks: ( + [ + { + type: "header", + text: {type: "plain_text", text: "πŸš€ Release Approval Needed - \($ENV.REPOSITORY)", emoji: true} + }, + { + type: "section", + text: {type: "mrkdwn", text: "A new release is ready for approval in the `\($ENV.REPOSITORY)` repository."} + }, + { + type: "section", + fields: ( + [{type: "mrkdwn", text: "*Repository:*\n"}] + + (if $ENV.PRS_MRKDWN != "" then [{type: "mrkdwn", text: "*Includes:*\n\($ENV.PRS_MRKDWN)"}] else [] end) + ) + } + ] + + (if $ENV.BLOCKED_MRKDWN != "" then [ + { + type: "section", + text: {type: "mrkdwn", text: "β›” *Cannot approve this release:* \($ENV.BLOCKED_MRKDWN)"} + }, + { + type: "context", + elements: [{type: "mrkdwn", text: "Authors of the PRs in this push, and whoever queued them, are excluded. The release fails if one of them approves."}] + } + ] else [] end) + + [ + { + type: "section", + text: {type: "mrkdwn", text: " Please review and approve the release."} + }, + { + type: "actions", + elements: [ + { + type: "button", + text: {type: "plain_text", text: "View Diff on GitHub", emoji: true}, + url: "https://github.com/\($ENV.REPOSITORY)/compare/\($ENV.LATEST_TAG)...\($ENV.DEFAULT_BRANCH)" + }, + { + type: "button", + text: {type: "plain_text", text: "Approve/Reject on GitHub", emoji: true}, + url: "https://github.com/\($ENV.REPOSITORY)/actions/runs/\($ENV.RUN_ID)", + style: "primary" + } + ] + } + ] + ) + }') + + delimiter="payload_$(openssl rand -hex 8)" + { + echo "payload<<$delimiter" + echo "$payload" + echo "$delimiter" + } >> "$GITHUB_OUTPUT" + - name: Send Slack notification id: notify uses: slackapi/slack-github-action@af78098f536edbc4de71162a307590698245be95 # v3.0.1 @@ -116,68 +223,7 @@ jobs: with: method: chat.postMessage token: ${{ secrets.slack_bot_token }} - payload: | - { - "channel": "${{ inputs.slack_channel_id }}", - "text": "πŸš€ Release Approval Needed for ${{ github.repository }}", - "blocks": [ - { - "type": "header", - "text": { - "type": "plain_text", - "text": "πŸš€ Release Approval Needed - ${{ github.repository }}", - "emoji": true - } - }, - { - "type": "section", - "text": { - "type": "mrkdwn", - "text": "A new release is ready for approval in the `${{ github.repository }}` repository." - } - }, - { - "type": "section", - "fields": [ - { - "type": "mrkdwn", - "text": "*Repository:*\n" - }, - ] - }, - { - "type": "section", - "text": { - "type": "mrkdwn", - "text": " Please review and approve the release." - } - }, - { - "type": "actions", - "elements": [ - { - "type": "button", - "text": { - "type": "plain_text", - "text": "View Diff on GitHub", - "emoji": true - }, - "url": "https://github.com/${{ github.repository }}/compare/${{ steps.get-info.outputs.latest_tag }}...${{ steps.get-info.outputs.default_branch }}" - }, - { - "type": "button", - "text": { - "type": "plain_text", - "text": "Approve/Reject on GitHub", - "emoji": true - }, - "url": "https://github.com/${{ github.repository }}/actions/runs/${{ github.run_id }}", - "style": "primary" - } - ] - } - ] - } + payload: ${{ steps.payload.outputs.payload }} - name: Send Slack failure event to PostHog if: ${{ steps.notify.outcome == 'failure' }} From ab96ac53c84b2b6ab55bd627d81e78bce288fa4b Mon Sep 17 00:00:00 2001 From: Ioannis J Date: Fri, 23 Oct 2026 10:00:48 +0300 Subject: [PATCH 2/2] fix(ci): pin the release approval guard and stop Slack mention parsing on PR titles --- .github/workflows/notify-approval-needed.yml | 23 +++----------------- 1 file changed, 3 insertions(+), 20 deletions(-) diff --git a/.github/workflows/notify-approval-needed.yml b/.github/workflows/notify-approval-needed.yml index 6f742aa..e2ff8b2 100644 --- a/.github/workflows/notify-approval-needed.yml +++ b/.github/workflows/notify-approval-needed.yml @@ -24,11 +24,6 @@ on: required: false type: string default: "creatordate" - guard_ref: - description: "Ref of PostHog/.github to fetch the release approval guard from. Point at a branch while iterating; callers may pin a SHA." - required: false - type: string - default: "main" secrets: slack_bot_token: required: true @@ -116,23 +111,11 @@ jobs: fi echo "default_branch=$DEFAULT_BRANCH" >> "$GITHUB_OUTPUT" - - name: Checkout release approval guard - id: guard-source - if: github.event_name == 'push' - continue-on-error: true - uses: actions/checkout@de0fac2e4500dabe0009e67214ff5f5447ce83dd # v6.0.2 - with: - repository: PostHog/.github - ref: ${{ inputs.guard_ref }} - path: .github-shared - sparse-checkout: .github/actions/release-approval-guard - persist-credentials: false - - name: List who cannot approve id: guard - if: github.event_name == 'push' && steps.guard-source.outcome == 'success' + if: github.event_name == 'push' continue-on-error: true - uses: ./.github-shared/.github/actions/release-approval-guard + uses: PostHog/.github/.github/actions/release-approval-guard@71adbfe4bf1dd9be4ab5debcb4a5b70789306d26 # pr-82 with: mode: report github-token: ${{ github.token }} @@ -170,7 +153,7 @@ jobs: type: "section", fields: ( [{type: "mrkdwn", text: "*Repository:*\n"}] - + (if $ENV.PRS_MRKDWN != "" then [{type: "mrkdwn", text: "*Includes:*\n\($ENV.PRS_MRKDWN)"}] else [] end) + + (if $ENV.PRS_MRKDWN != "" then [{type: "mrkdwn", verbatim: true, text: "*Includes:*\n\($ENV.PRS_MRKDWN)"}] else [] end) ) } ]