From b6e9615aed0f189eea9690f43c04857a3e422be4 Mon Sep 17 00:00:00 2001 From: Dotta <34892728+cryppadotta@users.noreply.github.com> Date: Fri, 6 Nov 2026 15:25:57 -0600 Subject: [PATCH] feat(ci): request advisory merge review after PR verification (#19480) ## Thinking Path > - Paperclip helps people manage AI agents and their work. > - Pull requests already run CI and automated code review. > - A completed check does not start a final advisory merge review. > - A private routine webhook can connect these two steps. > - The webhook credential must stay outside PR code and public logs. > - This pull request adds an optional notification from trusted branch code. > - The routine records a recommendation. A person keeps merge control. ## Linked Issues or Issue Description **What existing behavior does this improve?** It connects successful PR verification to an advisory Paperclip routine. **Subsystem affected** GitHub Actions and the existing routine webhook API. **Current behavior** A PR can finish verification without asking a routine for a final merge recommendation. **Proposed behavior** A trusted workflow reads the current PR and its complete check list. It sends the PR identity to a private webhook when required checks pass and other reported checks finish successfully, as skipped, or as neutral. The receiving routine runs the configured merge review skill and records its decision on its Paperclip task. **Reason and benefit** This gives maintainers an advisory decision after verification. It does not grant merge control to the routine. **Breaking changes** None. Missing secrets or delivery errors leave verification results unchanged. Searched open and closed PRs and issues for related work. No duplicate notification workflow was found. Related API work: #18058 and #18581. This change uses the routine API and does not alter chat credential setup. ## What Changed - Add a default-branch workflow for PR workflow completions, external checks, and commit statuses. - Use read permissions and a GitHub environment restricted to master for the private URL and bearer token. - Require successful `ci / verify`, `ci / e2e`, and additional branch-rule checks. Read every status page and refresh the PR before delivery. - Use a stable idempotency key for each PR, head, and base. Reject stale events and changed candidates. - Verify the two core check runs belong to the same real GitHub Actions suite for the unchanged PR caller and master-owned reusable CI. Reject forged names, renamed callers, and incomplete file listings. - Keep HTTP redirects disabled, bound request time, and print fixed delivery states only. - Add regression checks and document private setup, pause, rotation, and acceptance steps. - Grant commit-status read access. Add a separate PR API probe with the same read permissions and the real Actions token. The probe has no private environment or webhook secrets and sends no notification. ## Verification - Pass: `node --test .github/scripts/tests/notify-pr-merge-review.test.mjs` (21 tests after the security and token-permission fixes). - Pass: actionlint v1.7.12 on `.github/workflows/pr-merge-review.yml`. - Pass: live GitHub query of a current PR and its complete status list. This found a GraphQL field mismatch during development. The corrected query passed with 62 checks. - Pass: the configured bearer webhook returned `test_received` with `routineStarted=false` in setup mode. Readback confirmed an active routine and environment secrets restricted to the master branch. - Not run: `pnpm verify --explain` could not start because this checkout has no installed workspace dependencies. Required CI will supply broad coverage. - The first quality-gate run rejected a CI-only title because this change adds a script. The feature title passed the later gate. - The security review found that PR-controlled check names could forge eligibility. A focused regression failed before the fix and passed after suite, run, caller, and reusable-workflow checks were added. The corrected provenance query also passed against a completed historical CI run without sending a webhook. - Earlier CI at ec2c2dc failed in `workspace-recovery.test.ts` with a generated company-prefix collision, in `idle-domain-deadlines-postgres.test.ts` with a deadline snapshot mismatch and timeout, and in `CoordinatorTasks.test.tsx` with a route query mismatch. The browser shard also recorded a CSS preload test failure before cancellation. These failures remain recorded in [run 42245040952](https://github.com/paperclipai/paperclip/actions/runs/42245040952). This PR does not edit those production paths. Current master is merged and fresh CI is required; the earlier failures are not claimed resolved. - The repeated browser, general-server, and UI lanes passed at 715cee88ce. The earlier failures remain historical; their causes are not all established. The Runner package lane was still running before the next fix. - A reviewer found missing `statuses: read`, which the broader local token did not expose. The permission regression failed before the fix and passed after it. The new read-only API probe will test the query with the actual Actions token. - Pass at c3c84394b7: the real Actions-token API probe, typecheck, build, browser aggregate, security scan, and Greptile 5/5. No unresolved review threads remain. - The only remaining CI failure is a 15 second timeout in the existing retained-native-session-cleanup-work backlog test (server shard 6/12). This file and its production implementation are unchanged from the previous candidate whose lane passed. The one scoped rerun passed at the same head, and the required verify aggregate now passes. The timeout remains in [run 42250393645, attempt 1](https://github.com/paperclipai/paperclip/actions/runs/42250393645/attempts/1); its cause is not established. This is recovery evidence, not a claimed root-cause fix. - A nonblocking review suggestion to defer branch-rule lookup until a PR is matched remains a minor efficiency follow-up. It is not a correctness or security finding. - Pending: a real notification and resulting routine task after this workflow reaches master. The setup test did not start a PR review. ## Risks - This workflow uses secrets. Keep the environment restricted to the master branch. It runs only trusted branch code and does not read PR artifacts, caches, or executable files. - GitHub event timing and check visibility can race. Delivery is best effort. A missed event can omit a review; it cannot change CI results or merge a PR. - Optional skipped and neutral checks permit an assessment. They do not prove that a check ran. Required checks must succeed. The routine must assess unavailable scanner coverage and other evidence gaps before its recommendation. - Add any new PR verification workflow name to the completion trigger list. - PRs that change the primary CI caller, `.github/workflows/pr.yml`, need a manual advisory review until their caller change reaches master. This prevents a PR from replacing verification with a forged result. - This PR changes CI configuration. CODEOWNERS review and normal merge rules still apply. ## Model Used OpenAI GPT-6 family through Codex. The exact deployment model ID and context window are not exposed to this run. Used reasoning, tool use, and code execution. ## Checklist - [x] I have included a thinking path that traces from project context to this change - [x] I have specified the model used (with available version and capability details) - [x] I have checked ROADMAP.md and confirmed this PR does not duplicate planned core work - [x] I have searched GitHub for duplicate or related PRs and linked them above - [x] I have either (a) linked existing issues with `Fixes: #` / `Closes #` / `Refs #` OR (b) described the issue in-PR following the relevant issue template - [x] I have not referenced internal/instance-local Paperclip issues or links (only public GitHub `#NNN` / `github.com/paperclipai/paperclip` URLs) - [x] My branch name describes the change and contains no internal Paperclip ticket id or instance-derived details - [x] I have run tests locally and they pass - [x] I have added or updated tests where applicable - [x] I have updated relevant documentation to reflect my changes - [x] I have considered and documented any risks above - [x] All Paperclip CI gates are green - [x] Greptile is 5/5 with no open P2s, recommendations, or follow-ups - [x] I will address all Greptile and reviewer comments before requesting merge --------- Co-authored-by: Paperclip --- .github/scripts/notify-pr-merge-review.mjs | 177 ++++++++++++ .../tests/notify-pr-merge-review.test.mjs | 263 ++++++++++++++++++ .github/workflows/pr-merge-review.yml | 57 ++++ .github/workflows/pr-review-api-access.yml | 43 +++ doc/PR-MERGE-REVIEW-WEBHOOK.md | 85 ++++++ 5 files changed, 625 insertions(+) create mode 100644 .github/scripts/notify-pr-merge-review.mjs create mode 100644 .github/scripts/tests/notify-pr-merge-review.test.mjs create mode 100644 .github/workflows/pr-merge-review.yml create mode 100644 .github/workflows/pr-review-api-access.yml create mode 100644 doc/PR-MERGE-REVIEW-WEBHOOK.md 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.