diff --git a/.github/scripts/notify-pr-merge-review.mjs b/.github/scripts/notify-pr-merge-review.mjs new file mode 100644 index 00000000000..d8151919ba0 --- /dev/null +++ b/.github/scripts/notify-pr-merge-review.mjs @@ -0,0 +1,177 @@ +import { execFileSync } from "node:child_process"; +import { readFileSync } from "node:fs"; +import { fileURLToPath } from "node:url"; + +const repository = "paperclipai/paperclip"; +const shaPattern = /^[a-f0-9]{40}$/; +const minimumChecks = ["ci / verify", "ci / e2e"]; +const gh = (args) => JSON.parse(execFileSync("gh", args, { + encoding: "utf8", maxBuffer: 16 * 1024 * 1024, timeout: 30_000, + // Never allow command diagnostics or a response to reach the Actions log. + stdio: ["ignore", "pipe", "pipe"], +})); +const api = (endpoint) => gh(["api", endpoint]); +const pages = (endpoint) => gh(["api", "--paginate", "--slurp", endpoint]).flat(); +const query = `query($number:Int!, $endCursor:String) { + repository(owner:"paperclipai", name:"paperclip") { + pullRequest(number:$number) { + number url state isDraft headRefOid baseRefOid baseRefName changedFiles + statusCheckRollup { contexts(first:100, after:$endCursor) { + totalCount pageInfo { hasNextPage endCursor } + nodes { + __typename + ... on CheckRun { name status conclusion checkSuite { databaseId app { databaseId } + workflowRun { databaseId } } } + ... on StatusContext { context state } + } + } } + } + } +}`; + +export function snapshot(number, request = gh) { + let candidate; + let cursor; + const checks = []; + do { + const args = ["api", "graphql", "-f", `query=${query}`, "-F", `number=${number}`]; + if (cursor) args.push("-f", `endCursor=${cursor}`); + const result = request(args); + if (result.errors?.length) throw new Error("Incomplete check listing"); + const pr = result.data.repository.pullRequest; + if (candidate && (pr.headRefOid !== candidate.headRefOid || pr.baseRefOid !== candidate.baseRefOid)) { + throw new Error("Candidate changed during check pagination"); + } + candidate = pr; + const contexts = pr.statusCheckRollup.contexts; + checks.push(...contexts.nodes); + if (!contexts.pageInfo.hasNextPage) { + if (checks.length !== contexts.totalCount) throw new Error("Incomplete check listing"); + break; + } + const next = contexts.pageInfo.endCursor; + if (!next || next === cursor || checks.length >= contexts.totalCount) throw new Error("Incomplete check listing"); + cursor = next; + } while (checks.length < 10_000); + if (candidate.statusCheckRollup.contexts.pageInfo.hasNextPage) throw new Error("Check listing limit exceeded"); + return { ...candidate, statusCheckRollup: checks }; +} + +export function green(pr, requiredNames = minimumChecks) { + if (pr.state !== "OPEN" || pr.isDraft || pr.baseRefName !== "master" || + !shaPattern.test(pr.headRefOid) || !shaPattern.test(pr.baseRefOid)) return false; + const checks = pr.statusCheckRollup; + if (!Array.isArray(checks) || checks.length === 0) return false; + const relevant = checks; + const passed = (check) => check.__typename === "StatusContext" + ? check.state === "SUCCESS" + : check.__typename === "CheckRun" && check.status === "COMPLETED" && check.conclusion === "SUCCESS"; + // Optional skipped/neutral checks can mean no work or unavailable coverage. + // They permit assessment, not approval. Required checks must really succeed. + return relevant.every((check) => passed(check) || (check.__typename === "CheckRun" && + check.status === "COMPLETED" && ["SKIPPED", "NEUTRAL"].includes(check.conclusion))) && + [...new Set([...minimumChecks, ...requiredNames])].every((name) => + relevant.some((check) => (check.name ?? check.context) === name && passed(check))); +} + +export function trustedVerification(pr, { read = api, list = pages } = {}) { + // Names alone are forgeable in pull_request YAML. Require the real Actions + // suite for the unmodified PR caller and its master-owned reusable workflow. + const actionsChecks = pr.statusCheckRollup.filter((check) => + check.__typename === "CheckRun" && check.status === "COMPLETED" && check.conclusion === "SUCCESS" && + check.checkSuite?.app?.databaseId === 15368 && + Number.isSafeInteger(check.checkSuite?.workflowRun?.databaseId)); + const runIds = new Set(actionsChecks.filter((check) => minimumChecks.includes(check.name)) + .map((check) => check.checkSuite.workflowRun.databaseId)); + let trusted = false; + for (const id of runIds) { + const run = read(`/repos/${repository}/actions/runs/${id}`); + if (run.repository?.full_name !== repository || run.path !== ".github/workflows/pr.yml" || + run.event !== "pull_request" || run.head_sha !== pr.headRefOid || + run.status !== "completed" || run.conclusion !== "success") continue; + if (!minimumChecks.every((name) => actionsChecks.some((check) => check.name === name && + check.checkSuite.workflowRun.databaseId === id && check.checkSuite.databaseId === run.check_suite_id))) continue; + const reusable = run.referenced_workflows?.find((entry) => + entry.path === `${repository}/.github/workflows/pr-trusted.yml@master` && + entry.ref === "refs/heads/master" && shaPattern.test(entry.sha)); + if (reusable) { trusted = true; break; } + } + if (!trusted || !Number.isSafeInteger(pr.changedFiles) || pr.changedFiles < 0) return false; + const files = list(`/repos/${repository}/pulls/${pr.number}/files?per_page=100`); + // GitHub caps this API at 3,000 files. A partial listing cannot prove that + // the caller is unmodified. Do not execute or download any of these files. + return files.length === pr.changedFiles && files.every((file) => + typeof file.filename === "string" && ![file.filename, file.previous_filename].includes(".github/workflows/pr.yml")); +} + +export async function deliver(pr, { url, token, fetcher = fetch } = {}) { + if (!url || !token) return "unconfigured"; + try { + const target = new URL(url); + if (target.protocol !== "https:" || target.username || target.password || target.search || target.hash || + !/^\/api\/routine-triggers\/public\/[a-f0-9]+\/fire$/.test(target.pathname)) return "invalid_configuration"; + const response = await fetcher(target, { + method: "POST", redirect: "error", signal: AbortSignal.timeout(15_000), + headers: { + "Content-Type": "application/json", Authorization: `Bearer ${token}`, + "Idempotency-Key": `pr-review:${repository}:${pr.number}:${pr.headRefOid}:${pr.baseRefOid}`, + }, + // No PR title, body, comments, or executable instructions cross this boundary. + body: JSON.stringify({ variables: { + pr_url: `https://github.com/${repository}/pull/${pr.number}`, + pr_number: String(pr.number), head_sha: pr.headRefOid, base_sha: pr.baseRefOid, + } }), + }); + // Do not read the response body: a proxy can echo the URL or credentials. + await response.body?.cancel(); + return response.ok ? "delivered" : "not_delivered"; + } catch { + // Fetch errors can contain the private URL. Keep the result fixed and quiet. + return "not_delivered"; + } +} + +export async function notify(event, { read = api, list = pages, view = snapshot, send = deliver } = {}) { + if (event.repository?.full_name !== repository) return []; + const run = event.workflow_run; + if (run && (run.status !== "completed" || !["pull_request", "pull_request_target"].includes(run.event))) return []; + const sha = run?.head_sha ?? event.check_run?.head_sha ?? event.sha; + if (!shaPattern.test(sha)) return []; + // Resolve forks through the commit API too: workflow_run.pull_requests can be empty. + const associated = run?.pull_requests?.length ? run.pull_requests + : list(`/repos/${repository}/commits/${sha}/pulls?per_page=100`); + const required = read(`/repos/${repository}/rules/branches/master`) + .filter((rule) => rule.type === "required_status_checks") + .flatMap((rule) => rule.parameters.required_status_checks.map((check) => check.context)); + const outcomes = []; + for (const number of new Set(associated.map((pr) => pr.number))) { + if (!Number.isSafeInteger(number) || number <= 0) continue; + const pr = view(number); + // Target-context workflows use the base SHA. All other events must still + // describe this PR's current head; an old successful run cannot trigger it. + if ((run?.event !== "pull_request_target" && pr.headRefOid !== sha) || !green(pr, required)) continue; + const fresh = view(number); + if (fresh.headRefOid !== pr.headRefOid || fresh.baseRefOid !== pr.baseRefOid || + !green(fresh, required) || !trustedVerification(fresh, { read, list })) continue; + outcomes.push(await send(fresh)); + } + return outcomes; +} + +async function main() { + try { + const url = process.env.PAPERCLIP_PR_REVIEW_WEBHOOK_URL; + const token = process.env.PAPERCLIP_PR_REVIEW_WEBHOOK_TOKEN; + if (!url || !token) { + console.log("PR review notification is not configured; skipped."); + return; + } + const event = JSON.parse(readFileSync(process.env.GITHUB_EVENT_PATH, "utf8")); + const outcomes = await notify(event, { send: (pr) => deliver(pr, { url, token }) }); + console.log(outcomes.length ? `PR review notification: ${outcomes.join(", ")}.` : "No current green PR to notify."); + } catch { + console.log("PR review notification was unavailable; verification results are unchanged."); + } +} + +if (process.argv[1] && fileURLToPath(import.meta.url) === process.argv[1]) await main(); diff --git a/.github/scripts/tests/notify-pr-merge-review.test.mjs b/.github/scripts/tests/notify-pr-merge-review.test.mjs new file mode 100644 index 00000000000..dbb8dca94da --- /dev/null +++ b/.github/scripts/tests/notify-pr-merge-review.test.mjs @@ -0,0 +1,263 @@ +import assert from "node:assert/strict"; +import test from "node:test"; +import { readFileSync, mkdtempSync, writeFileSync, rmSync } from "node:fs"; +import { spawnSync } from "node:child_process"; +import { tmpdir } from "node:os"; +import path from "node:path"; +import { green, deliver, notify, snapshot, trustedVerification } from "../notify-pr-merge-review.mjs"; + +const head = "a".repeat(40); +const base = "b".repeat(40); +const check = (name, conclusion = "SUCCESS", extra = {}) => ({ + __typename: "CheckRun", name, status: "COMPLETED", conclusion, workflowName: "PR", + checkSuite: { databaseId: 8, app: { databaseId: 15368 }, workflowRun: { databaseId: 7 } }, + ...extra, +}); +const candidate = (extra = {}) => ({ + number: 42, state: "OPEN", isDraft: false, headRefOid: head, baseRefOid: base, + baseRefName: "master", changedFiles: 1, statusCheckRollup: [check("ci / verify"), check("ci / e2e")], ...extra, +}); +const event = (extra = {}) => ({ repository: { full_name: "paperclipai/paperclip" }, sha: head, ...extra }); +const trustedRun = (extra = {}) => ({ + id: 7, repository: { full_name: "paperclipai/paperclip" }, path: ".github/workflows/pr.yml", + event: "pull_request", head_sha: head, status: "completed", conclusion: "success", check_suite_id: 8, + referenced_workflows: [{ path: "paperclipai/paperclip/.github/workflows/pr-trusted.yml@master", + ref: "refs/heads/master", sha: base }], ...extra, +}); +const files = [{ filename: "README.md" }]; +const read = (endpoint) => endpoint.includes("/rules/") ? [] : trustedRun(); +const list = (endpoint) => endpoint.includes("/files?") ? files : [{ number: 42 }]; +const deps = (extra = {}) => ({ + read, list, view: () => candidate(), + send: async () => "delivered", ...extra, +}); + +test("reads every check page and fails closed on incomplete or changing listings", () => { + const page = (nodes, hasNextPage, extra = {}) => ({ data: { repository: { pullRequest: candidate({ + statusCheckRollup: { contexts: { totalCount: 2, nodes, + pageInfo: { hasNextPage, endCursor: "next" } } }, ...extra, + }) } } }); + let calls = 0; + const pr = snapshot(42, (args) => { + if (++calls === 1) return page([check("ci / verify")], true); + assert.ok(args.includes("endCursor=next")); + return page([check("ci / e2e")], false); + }); + assert.equal(green(pr), true); + const own = check("notify", "", { status: "IN_PROGRESS", workflowName: undefined, + checkSuite: { workflowRun: { workflow: { name: "PR merge review notification" } } } }); + const withOwn = snapshot(42, () => ({ data: { repository: { pullRequest: candidate({ + statusCheckRollup: { contexts: { totalCount: 3, nodes: [...candidate().statusCheckRollup, own], + pageInfo: { hasNextPage: false } } }, + }) } } })); + assert.equal(green(withOwn), false); + assert.throws(() => snapshot(42, () => page([check("ci / verify")], false)), /Incomplete/); + calls = 0; + assert.throws(() => snapshot(42, () => ++calls === 1 ? page([check("ci / verify")], true) + : page([check("ci / e2e")], false, { headRefOid: "d".repeat(40) })), /Candidate changed/); + assert.throws(() => snapshot(42, () => ({ errors: [{ message: "unavailable" }] })), /Incomplete/); +}); + +test("requires real success from both aggregate checks and additional repository rules", () => { + assert.equal(green(candidate()), true); + for (const state of ["FAILURE", "CANCELLED", "TIMED_OUT", "ACTION_REQUIRED", "NEUTRAL", "SKIPPED", ""]) { + assert.equal(green(candidate({ statusCheckRollup: [check("ci / verify", state), check("ci / e2e")] })), false, state); + } + assert.equal(green(candidate({ statusCheckRollup: [check("ci / verify")] })), false); + assert.equal(green(candidate(), ["extra required"]), false); + assert.equal(green(candidate({ statusCheckRollup: [...candidate().statusCheckRollup, check("extra required")] }), ["extra required"]), true); + assert.equal(green(candidate({ statusCheckRollup: [] })), false); +}); + +test("waits for pending checks and statuses, permits skipped optional routing lanes", () => { + for (const extra of [check("tests", "", { status: "IN_PROGRESS" }), check("tests", "FAILURE"), + { __typename: "StatusContext", context: "security", state: "PENDING" }, + { __typename: "StatusContext", context: "security", state: "ERROR" }]) { + assert.equal(green(candidate({ statusCheckRollup: [...candidate().statusCheckRollup, extra] })), false); + } + assert.equal(green(candidate({ statusCheckRollup: [...candidate().statusCheckRollup, + check("not selected", "SKIPPED"), { __typename: "StatusContext", context: "security", state: "SUCCESS" }] })), true); + assert.equal(green(candidate({ statusCheckRollup: [...candidate().statusCheckRollup, + check("optional scanner coverage", "NEUTRAL")] })), true); +}); + +test("workflow names cannot hide pending or failing checks", () => { + const extras = ["PR merge review notification", "CI profile"].map((workflowName) => + check("notify", "", { status: "IN_PROGRESS", workflowName })); + assert.equal(green(candidate({ statusCheckRollup: [...candidate().statusCheckRollup, ...extras] })), false); + assert.equal(green(candidate({ statusCheckRollup: [...candidate().statusCheckRollup, + check("another", "", { status: "QUEUED", workflowName: "Other verification" })] })), false); +}); + +test("rejects closed, draft, wrong-base, and malformed candidates", () => { + for (const extra of [{ state: "MERGED" }, { isDraft: true }, { baseRefName: "release" }, + { headRefOid: "invalid" }, { baseRefOid: "invalid" }]) assert.equal(green(candidate(extra)), false); +}); + +test("resolves fork PRs from the head commit when workflow payload has no PRs", async () => { + const seen = []; + const outcomes = await notify(event({ workflow_run: { + status: "completed", event: "pull_request", head_sha: head, pull_requests: [], + } }), deps({ list: (endpoint) => { seen.push(endpoint); return list(endpoint); } })); + assert.deepEqual(outcomes, ["delivered"]); + assert.match(seen[0], new RegExp(`/commits/${head}/pulls\\?per_page=100$`)); +}); + +test("reads additional required rules and holds until their check succeeds", async () => { + const outcomes = await notify(event(), deps({ read: () => [{ type: "required_status_checks", + parameters: { required_status_checks: [{ context: "new gate" }] } }] })); + assert.deepEqual(outcomes, []); +}); + +test("forged successful check names from a PR-controlled workflow cannot notify", async () => { + const input = deps({ read: (endpoint) => endpoint.includes("/rules/") ? [] : { + id: 7, path: ".github/workflows/attacker.yml", event: "pull_request", + head_sha: head, status: "completed", conclusion: "success", + } }); + assert.deepEqual(await notify(event(), input), []); +}); + +test("requires genuine suite, caller, revision, and master reusable workflow provenance", () => { + assert.equal(trustedVerification(candidate(), deps()), true); + for (const changes of [ + { path: ".github/workflows/attacker.yml" }, { event: "pull_request_target" }, + { head_sha: "c".repeat(40) }, { status: "in_progress" }, { conclusion: "failure" }, + { repository: { full_name: "attacker/repo" } }, { check_suite_id: 99 }, + { referenced_workflows: [] }, + { referenced_workflows: [{ path: "paperclipai/paperclip/.github/workflows/pr-trusted.yml@master", + ref: "refs/pull/42/merge", sha: base }] }, + ]) { + assert.equal(trustedVerification(candidate(), deps({ read: () => trustedRun(changes) })), false); + } + const wrongSuite = check("ci / verify", "SUCCESS", { + checkSuite: { databaseId: 99, app: { databaseId: 1 }, workflowRun: { databaseId: 7 } }, + }); + assert.equal(trustedVerification(candidate({ + statusCheckRollup: [wrongSuite, check("ci / e2e")], + }), deps()), false); +}); + +test("modified or renamed caller and truncated file listings cannot notify", async () => { + for (const fileList of [[], [{ filename: ".github/workflows/pr.yml" }], + [{ filename: "moved.yml", previous_filename: ".github/workflows/pr.yml" }]]) { + const dependencies = deps({ list: (endpoint) => endpoint.includes("/files?") + ? fileList : [{ number: 42 }] }); + assert.deepEqual(await notify(event(), dependencies), []); + } + assert.equal(trustedVerification(candidate({ changedFiles: undefined }), deps()), false); +}); + +test("rejects stale head events and a head, base, or check change during refresh", async () => { + assert.deepEqual(await notify(event({ sha: "c".repeat(40) }), deps()), []); + for (const extra of [{ headRefOid: "c".repeat(40) }, { baseRefOid: "d".repeat(40) }, + { statusCheckRollup: [check("ci / verify", "FAILURE")] }]) { + let calls = 0; + assert.deepEqual(await notify(event(), deps({ view: () => candidate(++calls === 1 ? {} : extra) })), []); + } +}); + +test("handles target-context workflow completions by refreshing the associated PR", async () => { + assert.deepEqual(await notify(event({ workflow_run: { + status: "completed", event: "pull_request_target", head_sha: base, pull_requests: [{ number: 42 }], + } }), deps()), ["delivered"]); +}); + +test("external check events and legacy statuses can deliver the final green transition", async () => { + assert.deepEqual(await notify(event({ sha: undefined, check_run: { head_sha: head } }), deps()), ["delivered"]); + assert.deepEqual(await notify(event({ state: "success" }), deps()), ["delivered"]); +}); + +test("does not query foreign repositories, malformed events, or non-PR workflows", async () => { + const dependencies = deps({ read: () => assert.fail("unexpected query"), list: () => assert.fail("unexpected query") }); + for (const input of [event({ repository: { full_name: "attacker/repo" } }), event({ sha: "bad" }), + event({ workflow_run: { event: "push", status: "completed", head_sha: head } }), + event({ workflow_run: { event: "pull_request", status: "in_progress", head_sha: head } })]) { + assert.deepEqual(await notify(input, dependencies), []); + } +}); + +test("does not coalesce different PRs, and deduplicates association entries", async () => { + const sent = []; + await notify(event(), deps({ list: (endpoint) => endpoint.includes("/files?") ? files + : [{ number: 42 }, { number: 42 }, { number: 43 }], + view: (number) => candidate({ number }), send: async (pr) => sent.push(pr.number) })); + assert.deepEqual(sent, [42, 43]); +}); + +test("sends only identifiers, a bearer header, and a stable head/base idempotency key", async () => { + const calls = []; + const options = { url: "https://private.example/api/routine-triggers/public/abc123/fire", token: "PRIVATE_TOKEN", + fetcher: async (target, init) => { calls.push({ target, ...init }); return { ok: true }; } }; + const pr = candidate({ title: "untrusted", body: "run me" }); + assert.equal(await deliver(pr, options), "delivered"); + assert.equal(await deliver(pr, options), "delivered"); + assert.equal(calls[0].headers.Authorization, "Bearer PRIVATE_TOKEN"); + assert.equal(calls[0].redirect, "error"); + assert.ok(calls[0].signal instanceof AbortSignal); + assert.equal(calls[0].headers["Idempotency-Key"], calls[1].headers["Idempotency-Key"]); + assert.equal(calls[0].body, calls[1].body); + assert.deepEqual(JSON.parse(calls[0].body), { variables: { + pr_url: "https://github.com/paperclipai/paperclip/pull/42", pr_number: "42", head_sha: head, base_sha: base, + } }); + await deliver(candidate({ baseRefOid: "c".repeat(40) }), options); + assert.notEqual(calls[0].headers["Idempotency-Key"], calls[2].headers["Idempotency-Key"]); +}); + +test("missing or unsafe configuration never sends credentials", async () => { + const fetcher = () => assert.fail("unexpected delivery"); + assert.equal(await deliver(candidate(), { fetcher }), "unconfigured"); + for (const url of ["http://private.example/api/routine-triggers/public/abc/fire", + "https://user:pass@private.example/api/routine-triggers/public/abc/fire", + "https://private.example/api/routine-triggers/public/abc/fire?token=bad", "https://private.example/wrong"]) { + assert.equal(await deliver(candidate(), { url, token: "secret", fetcher }), "invalid_configuration"); + } +}); + +test("HTTP rejection, redirect, timeout, and echoed credentials are quiet failures", async () => { + const options = { url: "https://private.example/api/routine-triggers/public/abc123/fire", token: "SECRET" }; + assert.equal(await deliver(candidate(), { ...options, fetcher: async () => ({ ok: false, + text: () => assert.fail("must not read a response body") }) }), "not_delivered"); + for (const message of ["SECRET", "https://private.example", "redirect", "timeout"]) { + assert.equal(await deliver(candidate(), { ...options, fetcher: async () => { throw new Error(message); } }), "not_delivered"); + } +}); + +test("command failures cannot print a private URL, token, or event payload", () => { + const directory = mkdtempSync(path.join(tmpdir(), "pr-notifier-test-")); + try { + const file = path.join(directory, "event.json"); + writeFileSync(file, "PRIVATE_EVENT_INVALID_JSON"); + const run = spawnSync(process.execPath, [".github/scripts/notify-pr-merge-review.mjs"], { + encoding: "utf8", env: { ...process.env, GITHUB_EVENT_PATH: file, + PAPERCLIP_PR_REVIEW_WEBHOOK_URL: "PRIVATE_URL_SENTINEL", PAPERCLIP_PR_REVIEW_WEBHOOK_TOKEN: "PRIVATE_TOKEN_SENTINEL" }, + }); + assert.equal(run.status, 0); + assert.doesNotMatch(run.stdout + run.stderr, /PRIVATE_/); + assert.match(run.stdout, /verification results are unchanged/); + } finally { rmSync(directory, { recursive: true, force: true }); } +}); + +test("workflow uses read permissions, a protected environment, trusted checkout, and optional delivery", () => { + const workflow = readFileSync(".github/workflows/pr-merge-review.yml", "utf8"); + assert.match(workflow, /environment: pr-merge-review/); + assert.match(workflow, /ref: \$\{\{ github.sha \}\}/); + assert.match(workflow, /persist-credentials: false/); + assert.match(workflow, /continue-on-error: true/); + for (const permission of ["actions", "checks", "contents", "pull-requests", "statuses"]) { + assert.ok(workflow.includes(` ${permission}: read`), `Missing ${permission}: read`); + } + assert.doesNotMatch(workflow, /: write|download-artifact|restore@|pull_request_target:/); + for (const name of ["PAPERCLIP_PR_REVIEW_WEBHOOK_URL", "PAPERCLIP_PR_REVIEW_WEBHOOK_TOKEN"]) { + assert.ok(workflow.includes(`secrets.${name}`)); + assert.ok(!workflow.includes(`vars.${name}`)); + } +}); + +test("API probe uses the same read permissions without private webhook access", () => { + const probe = readFileSync(".github/workflows/pr-review-api-access.yml", "utf8"); + for (const permission of ["actions", "checks", "contents", "pull-requests", "statuses"]) { + assert.ok(probe.includes(` ${permission}: read`)); + } + assert.match(probe, /snapshot\(Number\(process.env.PR_NUMBER\)\)/); + assert.doesNotMatch(probe, /\$\{\{ secrets\.|environment:|: write|deliver\(/); +}); diff --git a/.github/workflows/pr-merge-review.yml b/.github/workflows/pr-merge-review.yml new file mode 100644 index 00000000000..87c08abbc44 --- /dev/null +++ b/.github/workflows/pr-merge-review.yml @@ -0,0 +1,57 @@ +name: PR merge review notification + +on: + workflow_run: + workflows: + - PR + - Runner Windows + - Sentry SDK contract + - Docker Runner check + - commitperclip PR Review + - Storybook Visual + - Storybook Deploy + - CI work benchmark + - Incremental CI qualification + - Backup checkpoint qualification + - PR review API access + types: [completed] + # External check providers and legacy commit statuses can finish after CI. + check_run: + types: [completed] + status: + +permissions: + actions: read + checks: read + contents: read + pull-requests: read + statuses: read + +jobs: + notify: + if: >- + github.repository == 'paperclipai/paperclip' && + (github.event_name != 'workflow_run' || + github.event.workflow_run.event == 'pull_request' || + github.event.workflow_run.event == 'pull_request_target') + runs-on: ubuntu-latest + timeout-minutes: 5 + # Restrict this environment to master. Repository secrets would also be + # available to workflows changed by an in-repository PR. + environment: pr-merge-review + continue-on-error: true + steps: + # Only the trusted default-branch commit. Never use the PR SHA, caches, + # artifacts, dependencies, or executable files from a pull request. + - uses: actions/checkout@3d3c42e5aac5ba805825da76410c181273ba90b1 # v7 + with: + ref: ${{ github.sha }} + persist-credentials: false + sparse-checkout: .github/scripts + - name: Notify the advisory review routine + continue-on-error: true + env: + GH_TOKEN: ${{ github.token }} + PAPERCLIP_PR_REVIEW_WEBHOOK_URL: ${{ secrets.PAPERCLIP_PR_REVIEW_WEBHOOK_URL }} + PAPERCLIP_PR_REVIEW_WEBHOOK_TOKEN: ${{ secrets.PAPERCLIP_PR_REVIEW_WEBHOOK_TOKEN }} + run: node .github/scripts/notify-pr-merge-review.mjs diff --git a/.github/workflows/pr-review-api-access.yml b/.github/workflows/pr-review-api-access.yml new file mode 100644 index 00000000000..cdf916fa29e --- /dev/null +++ b/.github/workflows/pr-review-api-access.yml @@ -0,0 +1,43 @@ +name: PR review API access + +on: + pull_request: + paths: + - .github/scripts/notify-pr-merge-review.mjs + - .github/scripts/tests/notify-pr-merge-review.test.mjs + - .github/workflows/pr-merge-review.yml + - .github/workflows/pr-review-api-access.yml + +permissions: + actions: read + checks: read + contents: read + pull-requests: read + statuses: read + +jobs: + probe: + if: github.repository == 'paperclipai/paperclip' + runs-on: ubuntu-latest + timeout-minutes: 5 + # This job has no environment or private webhook secrets. It proves that + # the notifier's status query works with the actual read-only Actions token. + steps: + - uses: actions/checkout@3d3c42e5aac5ba805825da76410c181273ba90b1 # v7 + with: + persist-credentials: false + sparse-checkout: .github/scripts + - name: Read the PR status rollup with the Actions token + env: + GH_TOKEN: ${{ github.token }} + PR_NUMBER: ${{ github.event.pull_request.number }} + PR_HEAD_SHA: ${{ github.event.pull_request.head.sha }} + run: | + node --input-type=module <<'JS' + import assert from 'node:assert/strict'; + import { snapshot } from './.github/scripts/notify-pr-merge-review.mjs'; + const pr = snapshot(Number(process.env.PR_NUMBER)); + assert.equal(pr.headRefOid, process.env.PR_HEAD_SHA); + assert.ok(pr.statusCheckRollup.length > 0); + console.log('Actions token can read the complete PR status rollup.'); + JS diff --git a/doc/PR-MERGE-REVIEW-WEBHOOK.md b/doc/PR-MERGE-REVIEW-WEBHOOK.md new file mode 100644 index 00000000000..6526aca0999 --- /dev/null +++ b/doc/PR-MERGE-REVIEW-WEBHOOK.md @@ -0,0 +1,85 @@ +# Advisory PR review notifications + +The optional `PR merge review notification` workflow sends a private webhook +after a current, open, non-draft PR targeting `master` passes verification. +It does not approve or merge a PR. Webhook failures do not change PR CI results. + +## Private configuration + +Create a GitHub Actions environment named `pr-merge-review`. Restrict its +deployment branch policy to the `master` branch only, with no tag policy. +Keep these two values in **environment secrets**, not repository variables, +repository secrets, committed files, task comments, or workflow outputs: + +- `PAPERCLIP_PR_REVIEW_WEBHOOK_URL`: a Paperclip routine webhook URL. +- `PAPERCLIP_PR_REVIEW_WEBHOOK_TOKEN`: its separate bearer signing secret. + +Use a webhook trigger with `signingMode: bearer`. The routine should use +`always_enqueue` concurrency so separate PRs cannot be coalesced into one task. +Define required text variables `pr_url`, `pr_number`, `head_sha`, and `base_sha`. +The notifier sends only those identifiers. The receiving agent must treat them +as untrusted data and verify the repository, PR identity, and current revision. + +Give the routine instructions to run `pr-merge-review` and publish its MERGE, +HOLD, or DO NOT MERGE recommendation on the routine execution's Paperclip task. +Require a fresh check of the head, base, reviews, required checks, and task gates. +Keep GitHub approval, merge, auto-merge, and public-comment actions disabled in +the instructions for this advisory phase. Configure the routine in Paperclip; +do not put instance-specific URLs or credentials in this repository. + +## Delivery and verification + +The notification workflow runs trusted default-branch code. It never checks out +PR code or downloads PR artifacts or caches. It reacts to the repository's PR +workflow completions, external check completions, and legacy commit statuses. +When adding another PR verification workflow, add its name to `workflow_run`. + +The separate `PR review API access` workflow checks notifier changes using +the real read-only Actions token. It has no private environment or webhook +credentials and sends no webhook. Both workflows explicitly grant +`actions`, `checks`, `contents`, `pull-requests`, and `statuses` read access. +The GraphQL status rollup needs both check and commit-status permissions. + +The notifier reads the current PR's status rollup and branch rules. Both +`ci / verify` and `ci / e2e`, plus any additional required checks, must succeed. +Other checks must succeed or finish as skipped or neutral. Optional skipped or +neutral checks permit an assessment; they do not prove successful verification. +For example, a neutral scanner can mean coverage is unavailable. The advisory +routine evaluates these evidence gaps and reviews before recommending merge. +Workflow names do not exclude checks. The notifier and CI profile exporter run +on the default branch, so their own checks do not belong to the PR revision. + +Check names alone are not trusted. Both core aggregates must belong to the same +GitHub Actions suite and a successful run of the repository's actual +`.github/workflows/pr.yml`. Its run metadata must reference +`pr-trusted.yml@master`. The complete PR file list must prove that the caller +`pr.yml` was not changed or renamed. A missing or truncated list stops delivery. +PRs that change this caller need a manual advisory review until their caller +change reaches the trusted branch. Other workflow changes can still be reviewed. + +A final refresh rejects head or base changes during evaluation. Paperclip's +idempotency receipt deduplicates events for the same PR, head, and base. A new +base can produce a new review. The routine must refresh again before its final +recommendation. Event delivery and host check visibility can race; notifications +are best effort, with no polling or delivery guarantee. + +The HTTP request uses TLS, a bearer header, no redirects, and a 15 second timeout. +Logs contain fixed delivery states only. Request and response bodies, URLs, +tokens, and raw exceptions are never logged. Missing secrets skip delivery. + +Run the focused regression checks with: + +```sh +node --test .github/scripts/tests/notify-pr-merge-review.test.mjs +``` + +For setup verification, use Paperclip's webhook `setupPending` test mode before +activation. This verifies authentication and ingress without starting a review. +After the workflow is merged to `master`, inspect a real successful workflow +delivery and its linked routine task to prove end-to-end operation. A saved +configuration or a setup test alone does not prove that journey. + +To pause reviews, pause the routine in Paperclip or disable its trigger. To stop +notifications, disable this workflow or remove the two environment secrets. +Rotate the trigger signing secret in Paperclip and replace the token environment +secret together. Keep the URL secret even though it has no embedded bearer token.