From 08da89dce0646e85186eb33f308c869088f85f4a Mon Sep 17 00:00:00 2001 From: Miguel Angel Simon Sierra Date: Fri, 18 Sep 2026 18:34:09 -0400 Subject: [PATCH] ci: require before and after captures on studio and player PRs --- .github/CODEOWNERS | 3 + .github/workflows/pr-captures.yml | 32 ++++ package.json | 2 +- scripts/check-pr-captures.mjs | 243 +++++++++++++++++++++++++++++ scripts/check-pr-captures.test.mjs | 237 ++++++++++++++++++++++++++++ 5 files changed, 516 insertions(+), 1 deletion(-) create mode 100644 .github/CODEOWNERS create mode 100644 .github/workflows/pr-captures.yml create mode 100644 scripts/check-pr-captures.mjs create mode 100644 scripts/check-pr-captures.test.mjs diff --git a/.github/CODEOWNERS b/.github/CODEOWNERS new file mode 100644 index 0000000000..96f0c7a7d6 --- /dev/null +++ b/.github/CODEOWNERS @@ -0,0 +1,3 @@ +/scripts/check-pr-captures.mjs @miguel-heygen +/scripts/check-pr-captures.test.mjs @miguel-heygen +/.github/workflows/pr-captures.yml @miguel-heygen diff --git a/.github/workflows/pr-captures.yml b/.github/workflows/pr-captures.yml new file mode 100644 index 0000000000..81d2b899ad --- /dev/null +++ b/.github/workflows/pr-captures.yml @@ -0,0 +1,32 @@ +name: PR captures + +permissions: + contents: read + pull-requests: read + +# Runs on every PR (no path filter) so it can be a required check; the script passes when neither package changed. +on: + pull_request: + types: [opened, synchronize, reopened, edited] + branches: [main] + +concurrency: + group: pr-captures-${{ github.event.pull_request.number }} + cancel-in-progress: true + +jobs: + captures: + name: Studio and player captures + runs-on: ubuntu-latest + timeout-minutes: 3 + steps: + - uses: actions/checkout@34e114876b0b11c390a56381ad16ebd13914f8d5 # v4 + with: + fetch-depth: 0 + - name: Require Before and After captures + env: + PR_BODY: ${{ github.event.pull_request.body }} + PR_NUMBER: ${{ github.event.pull_request.number }} + BASE_REF: ${{ github.event.pull_request.base.ref }} + HEAD_SHA: ${{ github.event.pull_request.head.sha }} + run: node scripts/check-pr-captures.mjs --base "origin/$BASE_REF" --head "$HEAD_SHA" diff --git a/package.json b/package.json index a1a6f8a6b4..2fbb9f79a9 100644 --- a/package.json +++ b/package.json @@ -49,7 +49,7 @@ "player:perf": "bun run --filter @hyperframes/player perf", "format:check": "oxfmt --check .", "knip": "knip", - "test:scripts": "node --import tsx --test scripts/animejs-v4-guidance.test.mjs scripts/check-tracked-artifacts.test.mjs scripts/check-no-main-deletions.test.mjs scripts/check-docs-snippet-motion.test.mjs scripts/registry-target-paths.test.mjs scripts/check-workspace-contracts.test.mjs scripts/check-package-cycles.test.mjs scripts/check-cli-process-ownership.test.mjs scripts/check-large-files.test.mjs scripts/package-subpaths.test.mjs scripts/validate-release-channel.test.mjs scripts/publish-workflow.test.mjs scripts/install-workspace-dependencies.test.mjs scripts/draft-changelog.test.ts scripts/set-version.test.ts scripts/release-prepare.test.ts scripts/cli-options.test.ts scripts/changelog-weekly.test.ts scripts/claude-plugin-compression.test.ts scripts/catalog-payload-assets.test.ts scripts/host-registry-assets.test.ts scripts/catalog-preview-temp.test.ts scripts/catalog-hosted-files.test.ts scripts/player-cdn-pin.test.ts scripts/studio-runtime-smoke.test.mjs scripts/verify-packed-manifests.test.mjs scripts/lint-skills.test.mjs scripts/creator-editing-recipes.test.mjs packages/gcp-cloud-run/check-dockerfile-workspaces.test.mjs packages/core/scripts/writeGeneratedFile.test.ts && vitest run scripts/catalog/", + "test:scripts": "node --import tsx --test scripts/animejs-v4-guidance.test.mjs scripts/check-tracked-artifacts.test.mjs scripts/check-no-main-deletions.test.mjs scripts/check-pr-captures.test.mjs scripts/check-docs-snippet-motion.test.mjs scripts/registry-target-paths.test.mjs scripts/check-workspace-contracts.test.mjs scripts/check-package-cycles.test.mjs scripts/check-cli-process-ownership.test.mjs scripts/check-large-files.test.mjs scripts/package-subpaths.test.mjs scripts/validate-release-channel.test.mjs scripts/publish-workflow.test.mjs scripts/install-workspace-dependencies.test.mjs scripts/draft-changelog.test.ts scripts/set-version.test.ts scripts/release-prepare.test.ts scripts/cli-options.test.ts scripts/changelog-weekly.test.ts scripts/claude-plugin-compression.test.ts scripts/catalog-payload-assets.test.ts scripts/host-registry-assets.test.ts scripts/catalog-preview-temp.test.ts scripts/catalog-hosted-files.test.ts scripts/player-cdn-pin.test.ts scripts/studio-runtime-smoke.test.mjs scripts/verify-packed-manifests.test.mjs scripts/lint-skills.test.mjs scripts/creator-editing-recipes.test.mjs packages/gcp-cloud-run/check-dockerfile-workspaces.test.mjs packages/core/scripts/writeGeneratedFile.test.ts && vitest run scripts/catalog/", "typecheck:scripts": "tsc --noEmit -p scripts/tsconfig.json", "test:skills": "node --test 'skills/**/*.test.mjs'", "generate:previews": "tsx scripts/generate-template-previews.ts", diff --git a/scripts/check-pr-captures.mjs b/scripts/check-pr-captures.mjs new file mode 100644 index 0000000000..10c624798d --- /dev/null +++ b/scripts/check-pr-captures.mjs @@ -0,0 +1,243 @@ +#!/usr/bin/env node +// Fail a PR touching packages/studio or packages/player unless its body has Before and After sections with media. +// usage: node scripts/check-pr-captures.mjs --base origin/main --head ; the body arrives in the env (see main). + +import { execFileSync } from "node:child_process"; +import { realpathSync } from "node:fs"; +import { pathToFileURL } from "node:url"; + +export const WATCHED_PREFIXES = ["packages/studio/", "packages/player/"]; +export const NO_VISIBLE_CHANGE_MAX_LINES = 20; +export const VISUAL_EXTENSIONS = [".tsx", ".css", ".html"]; + +const MEDIA_PATH = /\.(?:png|jpe?g|gif|webp|svg|mp4|mov|webm)$/i; +const ATTACHMENT_PATH = /^\/user-attachments\/assets\/[\w-]+/; +const IMAGE_EMBED = + /!\[[^\]]*\]\(\s*https?:\/\/|<(?:img|video|source)\b[^>]*\bsrc=["']https?:\/\//i; +const ANY_URL = /https?:\/\/[^\s)"'<>\]]+/gi; + +function parseUrl(raw) { + try { + return new URL(raw); + } catch { + return null; + } +} + +const isAttachmentUrl = (url) => + url.hostname === "github.com" && ATTACHMENT_PATH.test(url.pathname); + +function isMediaUrl(raw) { + const url = parseUrl(raw); + return url !== null && (isAttachmentUrl(url) || MEDIA_PATH.test(url.pathname)); +} + +const TEST_FILE = /\.(?:test|spec)\.[jt]sx?$/; + +const CAPTURE_TITLES = { + before: /^before(?:\s*[:(].*|\s+[-–—]\s.*)?$/i, + after: /^after(?:\s*[:(].*|\s+[-–—]\s.*)?$/i, + noVisibleChange: /^no visible change\b/i, +}; + +const isCapture = (title) => Object.values(CAPTURE_TITLES).some((re) => re.test(title)); + +const SETEXT = [ + [/^=+\s*$/, "#"], + [/^-{2,}\s*$/, "##"], +]; + +/** GitHub hides an unclosed comment to the end of the body; repeat so removal cannot join fragments into a new one. */ +function stripComments(body) { + let text = body; + let previous; + do { + previous = text; + text = text.replace(/|$)/g, ""); + } while (text !== previous); + return text; +} + +const FENCE = /^ {0,3}(`{3,}|~{3,})(.*)$/; + +const closesFence = (open, line) => { + const [, marker = "", rest = ""] = FENCE.exec(line) ?? []; + return marker[0] === open[0] && marker.length >= open.length && rest.trim() === ""; +}; + +const openingMarker = (line) => FENCE.exec(line)?.[1] ?? null; + +/** The open fence marker after this line (CommonMark: same character, at least as long, to close), or null. */ +const nextFence = (open, line) => { + if (open) return closesFence(open, line) ? null : open; + return openingMarker(line); +}; + +function blankFences(lines) { + let open = null; + return lines.map((line) => { + const before = open; + open = nextFence(open, line); + return before || open ? "" : line; + }); +} + +const setextPrefix = (underline) => SETEXT.find(([re]) => re.test(underline))?.[1]; + +function promoteSetext(lines) { + const padded = [...lines, ""]; + return lines.map((line, i) => { + const prefix = isCapture(line.trim()) ? setextPrefix(padded[i + 1]) : undefined; + return prefix ? `${prefix} ${line.trim()}` : line; + }); +} + +/** Drop HTML comments and fenced code, and promote setext capture headings, so none can fake a section. */ +const normalize = (body) => promoteSetext(blankFences(stripComments(body).split(/\r?\n/))); + +/** Sections run to the next heading of the same or a higher level, or to the next capture heading. */ +export function parseSections(body) { + const lines = normalize(body); + const headings = []; + lines.forEach((line, index) => { + const match = /^(#{1,6})\s+(.+?)\s*#*\s*$/.exec(line); + if (match) headings.push({ level: match[1].length, title: match[2].trim(), index }); + }); + return headings.map((heading, i) => { + const end = headings + .slice(i + 1) + .find((next) => next.level <= heading.level || isCapture(next.title)); + return { + title: heading.title, + text: lines.slice(heading.index + 1, end ? end.index : lines.length).join("\n"), + }; + }); +} + +export function hasMedia(text) { + return IMAGE_EMBED.test(text) || (text.match(ANY_URL) ?? []).some(isMediaUrl); +} + +function findSection(sections, name) { + return sections.find((section) => CAPTURE_TITLES[name].test(section.title)); +} + +function parseRecord(record) { + const [added, deleted, ...rest] = record.split("\t"); + const binary = added === "-" || deleted === "-"; + const lines = binary ? NO_VISIBLE_CHANGE_MAX_LINES : Number(added) + Number(deleted); + return { path: rest.join("\t"), lines }; +} + +const isWatched = (path) => + path !== "" && WATCHED_PREFIXES.some((prefix) => path.startsWith(prefix)); + +/** Parse `git diff --numstat -z --no-renames`, keeping watched paths. A binary file counts as a full budget. */ +export const parseNumstat = (numstat) => + numstat + .split("\0") + .map(parseRecord) + .filter((file) => isWatched(file.path)); + +const isVisualFile = (path) => + VISUAL_EXTENSIONS.some((ext) => path.endsWith(ext)) && !TEST_FILE.test(path); + +/** Why a "No visible change" declaration does not hold for this diff; empty means it holds. */ +export function noVisibleChangeFailures(files) { + const failures = []; + const lines = files.reduce((sum, file) => sum + file.lines, 0); + if (lines >= NO_VISIBLE_CHANGE_MAX_LINES) { + failures.push( + `the diff changes ${lines} lines under packages/studio and packages/player; the limit is under ${NO_VISIBLE_CHANGE_MAX_LINES}`, + ); + } + const visual = files.filter((file) => isVisualFile(file.path)); + if (visual.length > 0) { + failures.push( + `the diff touches ${VISUAL_EXTENSIONS.join(", ")} files: ${visual.map((file) => file.path).join(", ")}`, + ); + } + return failures; +} + +function sectionProblem(section, name) { + if (!section) return `the body has no "${name}" heading`; + return hasMedia(section.text) ? null : `the "${name}" section has no image or video`; +} + +const captureProblems = (sections) => + [ + sectionProblem(findSection(sections, "before"), "Before"), + sectionProblem(findSection(sections, "after"), "After"), + ].filter(Boolean); + +function noVisibleChangeVerdict(sections, files) { + if (!findSection(sections, "noVisibleChange")) return { holds: false, problems: [] }; + const failures = noVisibleChangeFailures(files); + const problems = failures.map((failure) => `"No visible change" does not apply: ${failure}`); + return { holds: failures.length === 0, problems }; +} + +const PASS = { ok: true, problems: [] }; + +export function evaluate({ body, files }) { + if (files.length === 0) return PASS; + const sections = parseSections(body); + const captures = captureProblems(sections); + if (captures.length === 0) return PASS; + const verdict = noVisibleChangeVerdict(sections, files); + if (verdict.holds) return PASS; + return { ok: false, problems: [...verdict.problems, ...captures] }; +} + +export function attachCommand(prNumber) { + return `gh pr edit ${prNumber} --attach ./before.png --attach ./after.png`; +} + +function flag(args, name, fallback) { + const at = args.indexOf(name); + return at === -1 ? fallback : args[at + 1]; +} + +function readNumstat(base, head) { + try { + return execFileSync("git", ["diff", "--numstat", "-z", "--no-renames", `${base}...${head}`], { + encoding: "utf8", + }); + } catch (error) { + console.error(`cannot diff ${base}...${head}: ${error.message.trim()}`); + return process.exit(2); + } +} + +function printFailure(problems, prNumber) { + console.error( + "This PR changes packages/studio or packages/player, so its body must show the behaviour.", + ); + for (const problem of problems) console.error(` - ${problem}`); + console.error("\nAdd '## Before' and '## After' sections, each with an image or video, then:"); + console.error(` ${attachCommand(prNumber)}`); + console.error("Only the PR description counts; captures posted as comments are not read."); + console.error( + "Edit the body text first: gh pr edit --body-file replaces the body and drops attachments.", + ); + console.error( + `A change with no visible effect (under ${NO_VISIBLE_CHANGE_MAX_LINES} lines, no .tsx/.css/.html) may instead add a '## No visible change' section.`, + ); +} + +function main() { + const args = process.argv.slice(2); + const numstat = readNumstat(flag(args, "--base", "origin/main"), flag(args, "--head", "HEAD")); + const body = process.env.PR_BODY ?? ""; + const { ok, problems } = evaluate({ body, files: parseNumstat(numstat) }); + if (ok) { + console.log("packages/studio and packages/player: captures present, or nothing to show."); + return; + } + printFailure(problems, process.env.PR_NUMBER ?? ""); + process.exit(1); +} + +if (process.argv[1] && import.meta.url === pathToFileURL(realpathSync(process.argv[1])).href) + main(); diff --git a/scripts/check-pr-captures.test.mjs b/scripts/check-pr-captures.test.mjs new file mode 100644 index 0000000000..360ff23175 --- /dev/null +++ b/scripts/check-pr-captures.test.mjs @@ -0,0 +1,237 @@ +import { strict as assert } from "node:assert"; +import { spawnSync } from "node:child_process"; +import { copyFileSync, mkdtempSync } from "node:fs"; +import { tmpdir } from "node:os"; +import { join } from "node:path"; +import { test } from "node:test"; + +import { + attachCommand, + evaluate, + hasMedia, + parseNumstat, + parseSections, +} from "./check-pr-captures.mjs"; + +const ASSET = "https://github.com/user-attachments/assets/9173dafa-203a-447a-be3a-1234567890ab"; +const studio = (path, lines = 50) => ({ path, lines }); +const captured = `## Before\n![old](${ASSET})\n\n## After\n${ASSET}\n`; + +test("a PR that touches neither package needs nothing", () => { + assert.equal(evaluate({ body: "", files: [] }).ok, true); +}); + +test("Before and After each with an attachment pass", () => { + assert.equal(evaluate({ body: captured, files: [studio("packages/studio/src/A.tsx")] }).ok, true); +}); + +test("a missing After heading fails and names it", () => { + const { ok, problems } = evaluate({ + body: `## Before\n![x](${ASSET})`, + files: [studio("packages/player/src/a.ts")], + }); + assert.equal(ok, false); + assert.deepEqual(problems, ['the body has no "After" heading']); +}); + +test("a heading with no media fails and names the section", () => { + const body = `## Before\nlooked bad\n\n## After\n![x](${ASSET})`; + const { problems } = evaluate({ body, files: [studio("packages/studio/src/a.ts")] }); + assert.deepEqual(problems, ['the "Before" section has no image or video']); +}); + +test("media in the wrong section does not count", () => { + const body = `## What\n![x](${ASSET})\n## Before\nnone\n## After\nnone`; + assert.equal(evaluate({ body, files: [studio("packages/studio/src/a.ts")] }).ok, false); +}); + +test("a local file reference is not an attachment", () => { + assert.equal(hasMedia("![shot](./before.png)"), false); +}); + +test("an image link and a video link count", () => { + assert.equal(hasMedia("[clip](https://example.com/a/b.mp4?raw=1)"), true); + assert.equal(hasMedia("![alt](https://example.com/shot.png)"), true); + assert.equal(hasMedia(''), true); + assert.equal(hasMedia("[docs](https://example.com/page)"), false); +}); + +test("headings inside a code fence are ignored", () => { + const sections = parseSections("```\n## Before\n```\n## After\nx"); + assert.deepEqual( + sections.map((s) => s.title), + ["After"], + ); +}); + +test("a nested heading stays inside its section", () => { + const sections = parseSections(`## Before\n### Wide\n${ASSET}\n## After\nz`); + assert.equal(hasMedia(sections[0].text), true); +}); + +test("No visible change passes for a small non-visual diff", () => { + const body = "## No visible change\nrename of an internal type"; + assert.equal(evaluate({ body, files: [studio("packages/studio/src/types.ts", 8)] }).ok, true); +}); + +test("No visible change fails at 20 lines and says why", () => { + const body = "## No visible change\nx"; + const { ok, problems } = evaluate({ body, files: [studio("packages/studio/src/a.ts", 20)] }); + assert.equal(ok, false); + assert.match(problems[0], /changes 20 lines .* under 20/); +}); + +test("No visible change fails on a .tsx file even when tiny, and names the file", () => { + const body = "## No visible change\nx"; + const { problems } = evaluate({ body, files: [studio("packages/studio/src/Button.tsx", 1)] }); + assert.match(problems[0], /touches .*Button\.tsx/); +}); + +test("both No visible change conditions are reported together", () => { + const body = "## No visible change\nx"; + const { problems } = evaluate({ body, files: [studio("packages/studio/src/a.css", 90)] }); + assert.equal(problems.filter((p) => p.startsWith('"No visible change"')).length, 2); +}); + +test("numstat keeps only watched paths and counts added plus deleted", () => { + const files = parseNumstat( + "3\t2\tpackages/studio/src/a.ts\0" + "9\t9\tdocs/x.md\0" + "1\t0\tpackages/player/src/b.ts\0", + ); + assert.deepEqual(files, [ + studio("packages/studio/src/a.ts", 5), + studio("packages/player/src/b.ts", 1), + ]); +}); + +test("a binary file spends the whole no-visible-change budget", () => { + const [file] = parseNumstat("-\t-\tpackages/studio/public/logo.png\0"); + assert.equal(file.lines, 20); +}); + +test("the failure prints the exact attach command", () => { + assert.equal(attachCommand(4127), "gh pr edit 4127 --attach ./before.png --attach ./after.png"); +}); + +const changed = [studio("packages/studio/src/a.ts")]; + +test("an image inside an HTML comment does not count", () => { + const body = `## Before\n\n## After\n${ASSET}`; + assert.deepEqual(evaluate({ body, files: changed }).problems, [ + 'the "Before" section has no image or video', + ]); +}); + +test("an image inside a code fence does not count", () => { + const body = "## Before\n```\n" + ASSET + "\n```\n## After\n" + ASSET; + assert.equal(evaluate({ body, files: changed }).ok, false); +}); + +test("one image cannot serve both sections when After is nested under Before", () => { + const body = `## Before\n### After\n${ASSET}`; + const { problems } = evaluate({ body, files: changed }); + assert.deepEqual(problems, ['the "Before" section has no image or video']); +}); + +test("Before and After with a qualifier count, other words do not", () => { + assert.equal( + evaluate({ body: `## Before (guides off)\n${ASSET}\n## After: on\n${ASSET}`, files: changed }) + .ok, + true, + ); + assert.equal( + evaluate({ + body: `## Before you merge\n${ASSET}\n## After the merge\n${ASSET}`, + files: changed, + }).ok, + false, + ); +}); + +test("setext headings are recognised", () => { + assert.equal( + evaluate({ body: `Before\n======\n${ASSET}\nAfter\n-----\n${ASSET}`, files: changed }).ok, + true, + ); +}); + +test("an embedded image counts without a media extension, a plain link to a page does not", () => { + assert.equal(hasMedia("![shot](https://example.com/shot)"), true); + assert.equal(hasMedia(''), true); + assert.equal(hasMedia("[shot](https://example.com/shot)"), false); +}); + +test("the attachment host must be github.com itself", () => { + assert.equal( + hasMedia("https://evil.example/https://github.com/user-attachments/assets/abc"), + false, + ); + assert.equal(hasMedia("https://github.com.evil.example/user-attachments/assets/abc"), false); + assert.equal(hasMedia("https://github.com/user-attachments/assets/abc-123"), true); +}); + +test("comment fragments cannot rebuild a comment or hide a section from the reader", () => { + const body = `## Before\n-- ![x](${ASSET}) -->\n## After\n${ASSET}`; + assert.equal(evaluate({ body, files: changed }).ok, false); + assert.equal( + evaluate({ body: `## Before\n${ASSET}\n## After\n