diff --git a/.github/scripts/generate-pr-comment.js b/.github/scripts/generate-pr-comment.js index dfcbf268f63e3..f0201c2ec6ead 100644 --- a/.github/scripts/generate-pr-comment.js +++ b/.github/scripts/generate-pr-comment.js @@ -3,7 +3,7 @@ /** * @description Generates the PR comment from the run's analysis results - * @input --analysis --base-stats --package-size --a11y --visual --preview --jobs --source-conclusion --run-url + * @input --analysis --base-stats --package-size --a11y --visual --visual-frames --preview --jobs --source-conclusion --run-url * @output Formatted markdown comment body to stdout, compact by design: this * head's preview first, the modified components (preview links, size, lines * of code, a mark per browser suite that ran; three rows visible, the rest @@ -66,6 +66,8 @@ if (baseStats) { const preview = readOptionalJson('preview'); const jobs = readOptionalJson('jobs'); const packageSize = readOptionalJson('package-size'); +// The visual frames publish-frames.mjs put on this PR's assets branch. +const visualFrames = readOptionalJson('visual-frames'); const visualVerdict = readOptionalJson('visual'); let a11yReport = null; try { @@ -125,6 +127,7 @@ const report = buildCompactReport({ a11yReport, visualVerdict, packageSize, + visualFrames, }); const footerLinksStr = runUrl ? ` | ${extLink('View full report', runUrl)}` diff --git a/.github/scripts/lib/pr-preview.mjs b/.github/scripts/lib/pr-preview.mjs index 41b69970a06bb..03fddfa6eb094 100644 --- a/.github/scripts/lib/pr-preview.mjs +++ b/.github/scripts/lib/pr-preview.mjs @@ -113,6 +113,35 @@ export async function headPreview({ : {state: 'unknown', sha}; } +/** + * The frames publish-frames.mjs published, written for the generator, only + * when their URL is this pull request's assets branch for this exact head and + * each frame is a `/.png` path; otherwise null. + */ +export function trustedFrames({frames, identity, inputs}) { + const expected = `https://raw.githubusercontent.com/${identity.baseRepository}/assets/pr-${identity.prNumber}/visual/${identity.headSha}/`; + let published; + try { + published = JSON.parse(frames?.published || '[]'); + } catch { + return null; + } + if ( + frames?.imageBase !== expected || + !Array.isArray(published) || + published.length === 0 || + !published.every(entry => + /^(before|after|diff)\/(?!\.+\.png$)[A-Za-z0-9._-]{1,240}\.png$/.test( + String(entry), + ), + ) + ) + return null; + const file = path.join(inputs, 'visual-frames.json'); + fs.writeFileSync(file, JSON.stringify({imageBase: expected, published})); + return file; +} + /** * The source run's jobs (latest attempt) with each step's conclusion, so the * report can say which jobs ran and which skipped their work by path. Null @@ -584,6 +613,7 @@ export async function reconcilePrComment({ metadataPath = 'pr-analysis/pr-meta.json', baseStatsPath = 'pr-size-base/size-base.json', packageSizePath = 'pr-analysis/package-size.json', + visualFrames = null, a11yPath = 'a11y/a11y-report.json', visualPath = 'trusted-visual/verdict.json', createIfMissing = true, @@ -648,6 +678,8 @@ export async function reconcilePrComment({ repo, runId: identity.sourceRunId, }); + // Frames only from this PR's assets branch for this exact head. + const framesPath = trustedFrames({frames: visualFrames, identity, inputs}); const previewPath = path.join(inputs, 'preview.json'); const jobsPath = path.join(inputs, 'jobs.json'); if (preview) fs.writeFileSync(previewPath, JSON.stringify(preview)); @@ -676,6 +708,7 @@ export async function reconcilePrComment({ // checks, or with the ci:run-a11y label); the comment says so. ...(fs.existsSync(a11yPath) ? ['--a11y', a11yPath] : []), ...(fs.existsSync(visualPath) ? ['--visual', visualPath] : []), + ...(framesPath ? ['--visual-frames', framesPath] : []), ...(preview ? ['--preview', previewPath] : []), ...(jobs ? ['--jobs', jobsPath] : []), '--source-conclusion', diff --git a/.github/scripts/lib/pr-preview.test.mjs b/.github/scripts/lib/pr-preview.test.mjs index 4b86724a1f9c9..89d472245b331 100644 --- a/.github/scripts/lib/pr-preview.test.mjs +++ b/.github/scripts/lib/pr-preview.test.mjs @@ -15,6 +15,7 @@ import { confirmSourceRunIdentity, PR_ANALYSIS_MARKER, reconcilePrComment, + trustedFrames, resolveWorkflowRunPullRequest, validateAnalysisMetadata, } from './pr-preview.mjs'; @@ -492,6 +493,7 @@ describe('trusted PR preview identity', () => { expect(writeCapableJobs).toEqual([ 'spec-only-reconcile', + 'frames', 'comment', 'report', ]); @@ -561,7 +563,13 @@ describe('trusted PR preview identity', () => { describe('PR Comment keeps its outcome off main', () => { // workflow_run attaches these jobs to main's latest commit, so a failing step // here would turn main red for a pull request's problem. - const LANE = ['resolve', 'spec-only-reconcile', 'comment', 'report']; + const LANE = [ + 'resolve', + 'spec-only-reconcile', + 'frames', + 'comment', + 'report', + ]; const OPTIONAL = new Set(['actions/download-artifact@v8']); it.each(LANE)('never fails the run from the %s job', name => { @@ -588,10 +596,20 @@ describe('PR Comment keeps its outcome off main', () => { it('reports the outcome on the head of the run the event names', () => { const report = PR_COMMENT_WORKFLOW.jobs.report; - expect(report.needs).toEqual(['resolve', 'spec-only-reconcile', 'comment']); + expect(report.needs).toEqual([ + 'resolve', + 'spec-only-reconcile', + 'frames', + 'comment', + ]); const [step] = report.steps; expect(step.env.HEAD_SHA).toBe('${{ github.event.workflow_run.head_sha }}'); - for (const name of ['resolve', 'spec-only-reconcile', 'comment']) { + for (const name of [ + 'resolve', + 'spec-only-reconcile', + 'frames', + 'comment', + ]) { expect(step.env.FAILED).toContain( `needs.${name}.outputs.failed == 'true'`, ); @@ -1358,3 +1376,67 @@ describe('PR comment preview reconciliation', () => { }, ); }); + +describe('visual frames', () => { + const identity = { + prNumber: 9171, + headSha: 'a'.repeat(40), + baseRepository: 'meta/astryx', + }; + const imageBase = `https://raw.githubusercontent.com/meta/astryx/assets/pr-9171/visual/${'a'.repeat(40)}/`; + const tmp = () => fs.mkdtempSync(path.join(os.tmpdir(), 'frames-')); + + it("passes frames only from this pull request's assets branch for this head", () => { + const published = JSON.stringify(['before/b.png', 'diff/b.png']); + const file = trustedFrames({ + frames: {imageBase, published}, + identity, + inputs: tmp(), + }); + expect(JSON.parse(fs.readFileSync(file, 'utf8'))).toEqual({ + imageBase, + published: ['before/b.png', 'diff/b.png'], + }); + for (const bad of [ + imageBase.replace('pr-9171', 'pr-1'), + imageBase.replace('a'.repeat(40), 'b'.repeat(40)), + 'https://evil.example/visual/', + ]) + expect( + trustedFrames({ + frames: {imageBase: bad, published}, + identity, + inputs: tmp(), + }), + ).toBeNull(); + }); + + it('refuses frame paths that are not before/after/diff PNGs', () => { + for (const published of [ + '["../x.png"]', + '["other/b.png"]', + '[]', + 'not json', + ]) + expect( + trustedFrames({ + frames: {imageBase, published}, + identity, + inputs: tmp(), + }), + ).toBeNull(); + }); + + it('gives only the frames job contents: write, with no persisted credentials', () => { + const {jobs} = PR_COMMENT_WORKFLOW; + for (const [name, job] of Object.entries(jobs)) + expect(job.permissions?.contents === 'write', name).toBe( + name === 'frames', + ); + const checkout = jobs.frames.steps.find(step => + String(step.uses).startsWith('actions/checkout@'), + ); + expect(checkout.with).toEqual({ref: 'main', 'persist-credentials': false}); + expect(jobs.comment.needs).toEqual(['resolve', 'frames']); + }); +}); diff --git a/.github/scripts/lib/pr-status.js b/.github/scripts/lib/pr-status.js index 0111a9d5dcb27..ba4c358b16606 100644 --- a/.github/scripts/lib/pr-status.js +++ b/.github/scripts/lib/pr-status.js @@ -283,14 +283,22 @@ function buildComponentTable({analysis, origin, a11yReport, visualVerdict}) { } /** - * The changed shots, largest change first: up to VISUAL_FRAME_LIMIT rows - * naming each shot and its change, inside one disclosure whose summary - * carries the counts. The run's visual report holds the frames. + * The changed shots, largest change first: up to VISUAL_FRAME_LIMIT rows, + * inside one disclosure whose summary carries the counts. With `frames` + * (publish-frames.mjs's `imageBase` and the `/.png` it published), + * each row shows its before/after/diff thumbnails, and only published frames; + * without it, each row names its change. */ -function buildVisualDeltas({verdict}) { +/** + * The changed shots in report order: changed by pixels changed, largest first, + * then added, then removed. The report lists the first VISUAL_FRAME_LIMIT, + * and publish-frames.mjs publishes frames for the same ones, so thumbnails + * and rows cannot drift apart. + */ +function visualShots(verdict) { if (!verdict || verdict.status === 'skipped' || verdict.status === 'failed') - return ''; - const all = [ + return []; + return [ ...[...(verdict.changes || [])] .sort((a, b) => Number(b.diffPixels ?? 0) - Number(a.diffPixels ?? 0)) .map(change => ({...change, kinds: ['before', 'after', 'diff']})), @@ -301,6 +309,10 @@ function buildVisualDeltas({verdict}) { kinds: ['before'], })), ]; +} + +function buildVisualDeltas({verdict, frames = null}) { + const all = visualShots(verdict); if (all.length === 0) return ''; const rows = all.slice(0, VISUAL_FRAME_LIMIT); const unshown = all.length - rows.length; @@ -321,8 +333,24 @@ function buildVisualDeltas({verdict}) { .join(', '); return variant ? `${story} (${variant})` : story; }; - const header = '| Component · story | Change |\n|---|---|\n'; - const line = row => `| ${label(row)} | ${changeText(row)} |`; + const published = new Set(frames?.published || []); + const base = + frames?.imageBase && published.size > 0 + ? `${String(frames.imageBase).replace(/\/+$/, '')}/` + : null; + const frame = (row, kind) => { + if (!row.kinds.includes(kind)) return '—'; + const file = `${kind}/${row.key}.png`; + if (!published.has(file)) return 'not published'; + return `${kind} frame`; + }; + const header = base + ? '| Component · story | Before | After | Diff |\n|---|---|---|---|\n' + : '| Component · story | Change |\n|---|---|\n'; + const line = row => + base + ? `| ${label(row)} | ${frame(row, 'before')} | ${frame(row, 'after')} | ${frame(row, 'diff')} |` + : `| ${label(row)} | ${changeText(row)} |`; // A shot key is `---__-`. const componentOf = row => String( @@ -432,6 +460,7 @@ function buildCompactReport({ a11yReport = null, visualVerdict = null, packageSize = null, + visualFrames = null, }) { const origin = preview?.state === 'ready' ? preview.url : preview?.aliasOrigin || ''; @@ -440,10 +469,11 @@ function buildCompactReport({ origin, a11yReport, visualVerdict, - })}${buildVisualDeltas({verdict: visualVerdict})}${packagesLine(packageSize)}${notRunLine({jobs, a11yReport, visualVerdict})}`; + })}${buildVisualDeltas({verdict: visualVerdict, frames: visualFrames})}${packagesLine(packageSize)}${notRunLine({jobs, a11yReport, visualVerdict})}`; } module.exports = { + visualShots, SUITES, VISUAL_FRAME_LIMIT, baseJobName, diff --git a/.github/scripts/lib/pr-status.test.mjs b/.github/scripts/lib/pr-status.test.mjs index 60b13cf5c5f0d..636d9c03659de 100644 --- a/.github/scripts/lib/pr-status.test.mjs +++ b/.github/scripts/lib/pr-status.test.mjs @@ -247,6 +247,36 @@ describe('buildVisualDeltas', () => { expect(section).toContain("3 more are in the run's visual report."); }); + it('shows thumbnails for the frames publish-frames.mjs published, and only those', () => { + const section = buildVisualDeltas({ + verdict: { + status: 'changed', + changes: [change('b', 50), change('a', 1)], + added: ['core-dialog--default__neutral-light'], + }, + frames: { + imageBase: 'https://raw.example/frames', + published: [ + 'before/b.png', + 'after/b.png', + 'diff/b.png', + 'after/core-dialog--default__neutral-light.png', + ], + }, + }); + expect(section).toContain('| Component · story | Before | After | Diff |'); + expect(section).toContain( + 'before frame', + ); + expect(section).toContain( + '| Card · a (neutral, light) | not published | not published | not published |', + ); + // An added shot has an after frame only. + expect(section).toContain( + '| — | after frame | — |', + ); + }); + it('is absent without changes, or when the visual run failed', () => { expect(buildVisualDeltas({verdict: {status: 'ok', changes: []}})).toBe(''); expect(buildVisualDeltas({verdict: null})).toBe(''); diff --git a/.github/scripts/lib/visual-format.js b/.github/scripts/lib/visual-format.js deleted file mode 100644 index 54501059c4094..0000000000000 --- a/.github/scripts/lib/visual-format.js +++ /dev/null @@ -1,128 +0,0 @@ -// Copyright (c) Meta Platforms, Inc. and affiliates. - -/** - * Shared visual-gate formatting for PR enrichment. - * Used by generate-pr-comment.js and test-pr-enrichment.js. - */ - -const {inline, num, safeUrl} = require('./report-text'); - -/** - * Build the visual-regression section of the PR comment. - * - * The section has to survive four states, and the awkward one is `skipped`: - * a check that quietly vanishes on the PRs most likely to break something - * visually is worse than no check, so a skip states its reason explicitly. - * - * @param {object|null} verdict - the gate's verdict.json, or null when the job did not run - * @param {string} [reportUrl] - immutable published report for this run - * @param {string} [imageUrl] - immutable raw-image base for inline evidence - * @returns {string} markdown - */ -function buildVisualSection(verdict, reportUrl, imageUrl) { - if (!verdict) return ''; - - // Verdict fields are report data: render them as literal inline text (and - // counts as numbers), never as markup — see report-text.js. - const safeReportUrl = reportUrl ? safeUrl(reportUrl) : null; - const link = safeReportUrl - ? ` View the report` - : ''; - const reportBase = safeReportUrl - ? `${safeReportUrl.replace(/\/+$/, '')}/` - : null; - // Inline images use raw branch URLs so a delayed Pages deployment cannot - // break evidence that is already present on gh-pages. - const imageBase = safeUrl(imageUrl) - ? `${safeUrl(imageUrl).replace(/\/+$/, '')}/` - : reportBase; - - if (verdict.status === 'skipped') { - return `### Visual Regression\n\n**Status:** Skipped — ${inline(verdict.reason)}${link}\n\n`; - } - if (verdict.status === 'failed') { - return `### Visual Regression\n\n**Status:** ${num(verdict.counts?.failed)} shot(s) could not be captured.${link}\n\n`; - } - const added = verdict.added ?? []; - const removed = verdict.removed ?? []; - if (!verdict.changes || verdict.changes.length === 0) { - if (added.length > 0 || removed.length > 0) { - const frames = imageBase - ? [ - ...added.map(key => ({key, kind: 'after'})), - ...removed.map(key => ({key, kind: 'before'})), - ] - .slice(0, 3) - .map(({key, kind}) => { - const safeKey = encodeURIComponent(String(key)); - const label = - kind === 'after' ? 'Added — After' : 'Removed — Before'; - return `

${label}
${label} visual regression frame

`; - }) - .join('\n') - : ''; - return `### Visual Regression\n\n**${added.length} added · ${removed.length} removed.**${link}\n\n${frames}\n\n`; - } - const compared = num(verdict.counts?.total); - return `### Visual Regression\n\n**Status:** No visual change across ${compared} compared shot(s).\n\n`; - } - - const rows = verdict.changes - .slice(0, 20) - .map( - change => - `| ${inline(change.component || change.title)} | ${inline(change.name)} | ${inline(change.theme)} | ${inline(change.mode)} | ${num(change.diffPixels).toLocaleString()} |`, - ) - .join('\n'); - - // Put the evidence where the decision is made. A link to an artifact ZIP is - // not a visual review: it makes the reviewer download, unzip, and match file - // names before they can answer whether the after is correct. The static - // report is published beside the PR preview, and its first three deltas - // render directly in the comment; the full report carries the rest + wipe UI. - const evidence = imageBase - ? verdict.changes - .slice(0, 3) - .map(change => { - const key = encodeURIComponent(String(change.key ?? '')); - if (!key) return ''; - const label = `${inline(change.component || change.title)} — ${inline(change.name)} — ${inline(change.theme)} ${inline(change.mode)}`; - return ` -
-${label} - - - - - - - -
BeforeAfterDiff
Before visual regression frameAfter visual regression framePixel difference frame
-
`; - }) - .filter(Boolean) - .join('\n') - : ''; - const more = - verdict.changes.length > 20 - ? `\n\n_and ${verdict.changes.length - 20} more._` - : ''; - - return `### Visual Regression - -**${verdict.changes.length} of ${num(verdict.counts?.total)} shot(s) changed.**${link} - -A change here is a question, not a failure: check whether the *after* is the -picture you intended. Record that review on the PR. Baseline maintenance is an -explicit dispatch of CI; this report never rewrites the baseline or adds a release gate. - -| component | story | theme | mode | pixels | -|---|---|---|---|---| -${rows}${more} - -${evidence} - -`; -} - -module.exports = {buildVisualSection}; diff --git a/.github/scripts/lib/visual-format.test.mjs b/.github/scripts/lib/visual-format.test.mjs deleted file mode 100644 index d5ba7f1719aaa..0000000000000 --- a/.github/scripts/lib/visual-format.test.mjs +++ /dev/null @@ -1,340 +0,0 @@ -// Copyright (c) Meta Platforms, Inc. and affiliates. - -import {describe, expect, it} from 'vitest'; -import {createRequire} from 'node:module'; - -const {buildVisualSection} = createRequire(import.meta.url)( - './visual-format.js', -); - -const verdict = (overrides = {}) => ({ - status: 'pass', - counts: { - total: 16, - unchanged: 16, - changed: 0, - added: 0, - removed: 0, - failed: 0, - }, - changes: [], - ...overrides, -}); - -describe('buildVisualSection', () => { - it('renders nothing when the job did not run', () => { - expect(buildVisualSection(null)).toBe(''); - }); - - it('says so plainly when nothing moved', () => { - expect(buildVisualSection(verdict())).toContain( - 'No visual change across 16 compared shot(s)', - ); - }); - - it('shows added-only evidence instead of claiming there was no visual change', () => { - const section = buildVisualSection( - verdict({ - status: 'changed', - counts: { - total: 20, - unchanged: 14, - changed: 0, - added: 1, - removed: 0, - failed: 0, - }, - added: ['new-shot'], - removed: [], - }), - 'https://example.com/report/', - 'https://example.com/images/', - ); - expect(section).toContain('1 added · 0 removed'); - expect(section).toContain('View the report'); - expect(section).toContain('https://example.com/images/after/new-shot.png'); - expect(section).not.toContain('No visual change'); - }); - - it('shows the before image for a removed-only bundle', () => { - const section = buildVisualSection( - verdict({ - status: 'changed', - counts: { - total: 0, - unchanged: 0, - changed: 0, - added: 0, - removed: 1, - failed: 0, - }, - added: [], - removed: ['old-shot'], - }), - 'https://example.com/report/', - 'https://example.com/images/', - ); - expect(section).toContain('0 added · 1 removed'); - expect(section).toContain('https://example.com/images/before/old-shot.png'); - }); - - it('states the reason for a skip, so a broad PR does not look like a pass', () => { - const section = buildVisualSection( - verdict({ - status: 'skipped', - reason: '900 shots exceeds the 240-shot budget', - }), - 'https://example.com/report/', - ); - expect(section).toContain('Skipped'); - expect(section).toContain('900 shots exceeds the 240-shot budget'); - expect(section).toContain('View the report'); - expect(section).not.toContain('/accept-visual'); - }); - - it('renders verdict strings as one-line literal text', () => { - const section = buildVisualSection( - verdict({ - status: 'skipped', - reason: 'first line\n## not a heading, just text', - }), - ); - expect(section).toContain( - 'first line ## not a heading, just <em>text</em>', - ); - expect(section).not.toContain('\n## not a heading'); - }); - - it('keeps a change row a single table row whatever the field contents', () => { - const section = buildVisualSection( - verdict({ - counts: { - total: 2, - unchanged: 1, - changed: 1, - added: 0, - removed: 0, - failed: 0, - }, - changes: [ - { - component: 'Cell | with pipes', - name: 'story\nnewline', - theme: 'y2k', - mode: 'light', - diffPixels: '1234', - }, - ], - }), - ); - expect(section).toContain( - '| Cell \\| with pipes | story newline | y2k | light | 1,234 |', - ); - }); - - it('drops a report link that is not a plain https URL', () => { - const good = buildVisualSection( - verdict({status: 'failed', counts: {total: 1, failed: 1}}), - 'https://example.com/report', - ); - expect(good).toContain('href="https://example.com/report"'); - const bad = buildVisualSection( - verdict({status: 'failed', counts: {total: 1, failed: 1}}), - 'report.html" onmouseover="x', - ); - expect(bad).not.toContain('View the report'); - }); - - it('lists each changed shot with the theme and mode it changed in', () => { - const section = buildVisualSection( - verdict({ - status: 'changed', - counts: { - total: 16, - unchanged: 14, - changed: 2, - added: 0, - removed: 0, - failed: 0, - }, - changes: [ - { - key: 'a', - component: 'Button', - name: 'Primary', - theme: 'y2k', - mode: 'light', - diffPixels: 1126, - }, - { - key: 'b', - component: 'Button', - name: 'Primary', - theme: 'y2k', - mode: 'dark', - diffPixels: 1401, - }, - ], - }), - ); - expect(section).toContain('2 of 16 shot(s) changed'); - expect(section).toContain('| Button | Primary | y2k | light | 1,126 |'); - }); - - it('frames a change as a question rather than a failure', () => { - const section = buildVisualSection( - verdict({ - status: 'changed', - counts: { - total: 2, - unchanged: 1, - changed: 1, - added: 0, - removed: 0, - failed: 0, - }, - changes: [ - { - key: 'a', - component: 'B', - name: 'S', - theme: 't', - mode: 'light', - diffPixels: 5, - }, - ], - }), - ); - expect(section).toMatch(/question, not a failure/); - }); - - it('caps the table and says how many were left out', () => { - const changes = Array.from({length: 25}, (_, index) => ({ - key: `k${index}`, - component: 'C', - name: 'S', - theme: 't', - mode: 'light', - diffPixels: index, - })); - const section = buildVisualSection( - verdict({status: 'changed', counts: {total: 25, changed: 25}, changes}), - ); - expect(section).toContain('and 5 more'); - }); - - it('embeds before, after, and diff images for changed shots', () => { - const section = buildVisualSection( - verdict({ - status: 'changed', - counts: { - total: 2, - unchanged: 1, - changed: 1, - added: 0, - removed: 0, - failed: 0, - }, - context: {runId: '123456', runAttempt: '2'}, - changes: [ - { - key: 'core-button--primary__y2k-light', - component: 'Button', - name: 'Primary', - theme: 'y2k', - mode: 'light', - diffPixels: 12, - }, - ], - }), - 'https://evidence.example/pr/123/visual/head/run/', - 'https://evidence.example/pr/123/visual/head/run/', - ); - expect(section).toContain( - 'https://evidence.example/pr/123/visual/head/run/before/core-button--primary__y2k-light.png', - ); - expect(section).toContain( - 'https://evidence.example/pr/123/visual/head/run/after/core-button--primary__y2k-light.png', - ); - expect(section).toContain( - 'https://evidence.example/pr/123/visual/head/run/diff/core-button--primary__y2k-light.png', - ); - expect(section).toContain('BeforeAfterDiff'); - expect(section).toContain('Record that review on the PR'); - expect(section).not.toContain('/accept-visual'); - }); - - it('does not offer acceptance before immutable evidence is published', () => { - const section = buildVisualSection( - verdict({ - status: 'changed', - counts: { - total: 2, - unchanged: 1, - changed: 1, - added: 0, - removed: 0, - failed: 0, - }, - context: {runId: '123456', runAttempt: '2'}, - changes: [ - { - key: 'a', - component: 'Button', - name: 'Primary', - theme: 'neutral', - mode: 'light', - diffPixels: 12, - }, - ], - }), - ); - expect(section).not.toContain('/accept-visual'); - expect(section).not.toContain('repository maintainer'); - }); - - it('embeds at most three deltas so the PR comment stays readable', () => { - const changes = Array.from({length: 5}, (_, index) => ({ - key: `shot-${index}`, - component: 'C', - name: 'S', - theme: 't', - mode: 'light', - diffPixels: 1, - })); - const section = buildVisualSection( - verdict({status: 'changed', counts: {total: 5, changed: 5}, changes}), - 'https://example.com/visual/', - ); - expect((section.match(/
/g) ?? []).length).toBe(3); - expect(section).not.toContain('/before/shot-3.png'); - }); - - it('reports a capture failure distinctly from a change', () => { - const section = buildVisualSection( - verdict({status: 'failed', counts: {total: 4, failed: 2, changed: 0}}), - ); - expect(section).toContain('2 shot(s) could not be captured'); - }); - - it('links the report when one was published', () => { - const section = buildVisualSection( - verdict({ - status: 'changed', - counts: {total: 1, changed: 1}, - changes: [ - { - key: 'a', - component: 'B', - name: 'S', - theme: 't', - mode: 'light', - diffPixels: 1, - }, - ], - }), - 'https://example.com/report/', - ); - expect(section).toContain('https://example.com/report/'); - }); -}); diff --git a/.github/scripts/visual-gate/publish-frames.mjs b/.github/scripts/visual-gate/publish-frames.mjs new file mode 100644 index 0000000000000..123ea201d2562 --- /dev/null +++ b/.github/scripts/visual-gate/publish-frames.mjs @@ -0,0 +1,227 @@ +#!/usr/bin/env node +// Copyright (c) Meta Platforms, Inc. and affiliates. + +/** + * @file Publishes the changed visual shots' frames for the PR report. + * @input `--input ` (publish-pr-report.mjs's output: verdict.json and + * re-encoded `before|after|diff/.png`), `--pr `, + * `--head-sha `, `--repository `, `--limit `, and a + * token in GITHUB_TOKEN. + * @output Commits up to `limit` (at most 12) changed shots' frames, PNG or + * WebP of at most 500 KB each by their own bytes, largest change first, + * to the pull request's `assets/pr-` branch (the path PR screenshots + * already use) under `visual//`, marking a branch it creates with + * MARKER so only such branches are pruned, and writes `image_base` (their + * raw URL prefix) and `published` (each frame's `/.png`) to + * GITHUB_OUTPUT. Writes nothing when no shot changed. + * @position The PR Comment workflow's frames job, after the visual report is + * prepared; the comment job renders the thumbnails. It writes only that + * branch, and only frames the trusted publisher re-encoded. + */ + +import fs from 'node:fs'; +import {createRequire} from 'node:module'; +import path from 'node:path'; +import process from 'node:process'; +import {fileURLToPath} from 'node:url'; + +// The report's own shot order and limit (lib/pr-status.js). +const {visualShots, VISUAL_FRAME_LIMIT} = createRequire(import.meta.url)( + '../lib/pr-status.js', +); +const KEY = /^(?!\.+$)[A-Za-z0-9._-]{1,240}$/; +/** Shots published per head, whatever `--limit` asks for: the report's rows. */ +export const MAX_SHOTS = VISUAL_FRAME_LIMIT; +export const MAX_FRAME_BYTES = 500 * 1024; + +/** A PNG or WebP no larger than MAX_FRAME_BYTES, by its own bytes. */ +export function isPublishableImage(bytes) { + if ( + !Buffer.isBuffer(bytes) || + bytes.length === 0 || + bytes.length > MAX_FRAME_BYTES + ) + return false; + const png = Buffer.from([0x89, 0x50, 0x4e, 0x47, 0x0d, 0x0a, 0x1a, 0x0a]); + if (bytes.subarray(0, 8).equals(png)) return true; + return ( + bytes.subarray(0, 4).toString('latin1') === 'RIFF' && + bytes.subarray(8, 12).toString('latin1') === 'WEBP' + ); +} + +/** + * The file that marks an assets branch as created by this script. The daily + * branch pruner deletes only marked branches, once their PR closes; a branch + * someone pushed by hand never gets the marker. + */ +export const MARKER = '.pr-report-frames'; + +/** The only ref this script writes: PR #'s assets branch. */ +export function assetsBranch(pr) { + if (!Number.isSafeInteger(pr) || pr <= 0) throw new Error('not a PR number'); + return `assets/pr-${pr}`; +} + +/** + * The frames to publish: the report's rows (visualShots), the first `limit` + * of them and at most MAX_SHOTS, and only shots whose key is a safe name. + */ +export function framesToPublish(verdict, limit) { + return visualShots(verdict) + .slice(0, Math.min(Number(limit) || 0, MAX_SHOTS)) + .filter(shot => KEY.test(String(shot.key ?? ''))) + .map(shot => ({key: shot.key, kinds: shot.kinds})); +} + +async function api(request, method, route, body) { + const response = await request(method, route, body); + if (response.status === 404) return null; + if (response.status >= 300) + throw new Error(`${method} ${route} failed: ${response.status}`); + return response.json(); +} + +/** + * One commit on `assets/pr-` holding the frames, on top of the branch when + * it exists. Returns the raw URL prefix of the published frames and each + * published frame's `/.png`, or null when none was published. + */ +export async function publishFrames({ + input, + pr, + headSha, + repository, + limit, + request, +}) { + const verdict = JSON.parse( + fs.readFileSync(path.join(input, 'verdict.json'), 'utf8'), + ); + const files = []; + for (const shot of framesToPublish(verdict, limit)) { + for (const kind of shot.kinds) { + const file = path.join(input, kind, `${shot.key}.png`); + if (!fs.existsSync(file)) continue; + const bytes = fs.readFileSync(file); + if (!isPublishableImage(bytes)) continue; + files.push({ + frame: `${kind}/${shot.key}.png`, + path: `visual/${headSha}/${kind}/${shot.key}.png`, + bytes, + }); + } + } + if (files.length === 0) return null; + if (!/^[0-9a-f]{40}$/.test(String(headSha))) + throw new Error('not a head SHA'); + const branch = assetsBranch(pr); + const ref = await api( + request, + 'GET', + `/repos/${repository}/git/ref/heads/${branch}`, + ); + const parent = ref?.object?.sha ?? null; + const parentCommit = parent + ? await api(request, 'GET', `/repos/${repository}/git/commits/${parent}`) + : null; + const tree = []; + if (!parent) { + const marker = await api( + request, + 'POST', + `/repos/${repository}/git/blobs`, + { + content: Buffer.from( + 'Created by the PR report; pruned when the PR closes.\n', + ).toString('base64'), + encoding: 'base64', + }, + ); + tree.push({path: MARKER, mode: '100644', type: 'blob', sha: marker.sha}); + } + for (const entry of files) { + const blob = await api(request, 'POST', `/repos/${repository}/git/blobs`, { + content: entry.bytes.toString('base64'), + encoding: 'base64', + }); + tree.push({path: entry.path, mode: '100644', type: 'blob', sha: blob.sha}); + } + const newTree = await api(request, 'POST', `/repos/${repository}/git/trees`, { + ...(parentCommit ? {base_tree: parentCommit.tree.sha} : {}), + tree, + }); + const commit = await api( + request, + 'POST', + `/repos/${repository}/git/commits`, + { + message: `Visual frames for #${pr} at ${headSha.slice(0, 7)}`, + tree: newTree.sha, + parents: parent ? [parent] : [], + }, + ); + if (parent) + await api( + request, + 'PATCH', + `/repos/${repository}/git/refs/heads/${branch}`, + { + sha: commit.sha, + }, + ); + else + await api(request, 'POST', `/repos/${repository}/git/refs`, { + ref: `refs/heads/${branch}`, + sha: commit.sha, + }); + return { + imageBase: `https://raw.githubusercontent.com/${repository}/${branch}/visual/${headSha}/`, + published: files.map(entry => entry.frame), + }; +} + +if (process.argv[1] === fileURLToPath(import.meta.url)) { + const flag = name => { + const index = process.argv.indexOf(`--${name}`); + return index === -1 ? null : process.argv[index + 1]; + }; + const pr = Number(flag('pr')); + const headSha = String(flag('head-sha') ?? ''); + const repository = String(flag('repository') ?? ''); + if (!Number.isSafeInteger(pr) || pr <= 0) + throw new Error('--pr must be a PR number'); + if (!/^[0-9a-f]{40}$/.test(headSha)) + throw new Error('--head-sha must be a full SHA'); + if (!/^[A-Za-z0-9_.-]+\/[A-Za-z0-9_.-]+$/.test(repository)) + throw new Error('--repository must be owner/repo'); + const token = process.env.GITHUB_TOKEN; + const request = (method, route, body) => + fetch(`https://api.github.com${route}`, { + method, + headers: { + authorization: `Bearer ${token}`, + accept: 'application/vnd.github+json', + 'content-type': 'application/json', + }, + body: body ? JSON.stringify(body) : undefined, + }); + const result = await publishFrames({ + input: path.resolve(flag('input')), + pr, + headSha, + repository, + limit: Number(flag('limit') ?? 12), + request, + }); + if (result && process.env.GITHUB_OUTPUT) + fs.appendFileSync( + process.env.GITHUB_OUTPUT, + `image_base=${result.imageBase}\npublished=${JSON.stringify(result.published)}\n`, + ); + console.log( + result + ? `Published ${result.published.length} frames under ${result.imageBase}` + : 'No changed shots to publish.', + ); +} diff --git a/.github/scripts/visual-gate/publish-frames.test.mjs b/.github/scripts/visual-gate/publish-frames.test.mjs new file mode 100644 index 0000000000000..4605b8a8efed1 --- /dev/null +++ b/.github/scripts/visual-gate/publish-frames.test.mjs @@ -0,0 +1,243 @@ +// Copyright (c) Meta Platforms, Inc. and affiliates. + +/** + * @file publish-frames.test.mjs + * Pins which visual frames the PR report publishes (the largest changes + * first, then added and removed shots, at most `limit`, safe keys only) and + * how: one commit on `assets/pr-`, on top of the branch when it exists. + */ + +import fs from 'node:fs'; +import os from 'node:os'; +import path from 'node:path'; +import {afterEach, describe, expect, it} from 'vitest'; +import { + MAX_FRAME_BYTES, + MAX_SHOTS, + MARKER, + assetsBranch, + framesToPublish, + isPublishableImage, + publishFrames, +} from './publish-frames.mjs'; + +const PNG = Buffer.concat([ + Buffer.from([0x89, 0x50, 0x4e, 0x47, 0x0d, 0x0a, 0x1a, 0x0a]), + Buffer.from('rest'), +]); + +const HEAD = 'a'.repeat(40); +const roots = []; +afterEach(() => { + for (const root of roots.splice(0)) + fs.rmSync(root, {recursive: true, force: true}); +}); + +function input(verdict) { + const root = fs.mkdtempSync(path.join(os.tmpdir(), 'frames-')); + roots.push(root); + fs.writeFileSync(path.join(root, 'verdict.json'), JSON.stringify(verdict)); + for (const kind of ['before', 'after', 'diff']) { + fs.mkdirSync(path.join(root, kind)); + for (const key of ['a', 'b', 'c', 'added']) + fs.writeFileSync(path.join(root, kind, `${key}.png`), PNG); + } + return root; +} + +function fakeGitHub({branchExists}) { + const calls = []; + const request = async (method, route, body) => { + calls.push({method, route, body}); + const json = data => ({status: 200, json: async () => data}); + if (method === 'GET' && route.includes('/git/ref/')) + return branchExists + ? json({object: {sha: 'parent'}}) + : {status: 404, json: async () => ({})}; + if (method === 'GET' && route.includes('/git/commits/')) + return json({tree: {sha: 'parent-tree'}}); + if (route.endsWith('/git/blobs')) + return json({sha: `blob-${calls.length}`}); + if (route.endsWith('/git/trees')) return json({sha: 'tree'}); + if (route.endsWith('/git/commits')) return json({sha: 'commit'}); + return json({}); + }; + return {calls, request}; +} + +describe('framesToPublish', () => { + it('takes the largest changes first, then added and removed shots, up to the limit, and never an unsafe key', () => { + const verdict = { + changes: [ + {key: 'a', diffPixels: 1}, + {key: 'b', diffPixels: 9}, + {key: '../escape', diffPixels: 99}, + ], + added: ['added'], + removed: ['gone'], + }; + // The unsafe key keeps its report row (shown as not published). + expect(framesToPublish(verdict, 3).map(shot => shot.key)).toEqual([ + 'b', + 'a', + ]); + expect(framesToPublish(verdict, 5).map(shot => shot.key)).toEqual([ + 'b', + 'a', + 'added', + 'gone', + ]); + expect(framesToPublish({...verdict, status: 'skipped'}, 3)).toEqual([]); + }); +}); + +describe('the same shots as the report', () => { + it("publishes exactly the report's rows, in its order", async () => { + const {createRequire} = await import('node:module'); + const {visualShots, VISUAL_FRAME_LIMIT} = createRequire(import.meta.url)( + '../lib/pr-status.js', + ); + const verdict = { + status: 'changed', + changes: Array.from({length: 20}, (_, i) => ({ + key: `s${i}`, + diffPixels: i, + })), + added: ['new-shot'], + }; + expect(framesToPublish(verdict, 100).map(shot => shot.key)).toEqual( + visualShots(verdict) + .slice(0, VISUAL_FRAME_LIMIT) + .map(shot => shot.key), + ); + }); +}); + +describe('image and ref limits', () => { + it('accepts only PNG or WebP bytes no larger than 500 KB', () => { + expect(isPublishableImage(PNG)).toBe(true); + const webp = Buffer.concat([ + Buffer.from('RIFF'), + Buffer.alloc(4), + Buffer.from('WEBPVP8 '), + ]); + expect(isPublishableImage(webp)).toBe(true); + expect(isPublishableImage(Buffer.from(''))).toBe( + false, + ); + expect(isPublishableImage(Buffer.from('GIF89a'))).toBe(false); + expect( + isPublishableImage(Buffer.concat([PNG, Buffer.alloc(MAX_FRAME_BYTES)])), + ).toBe(false); + }); + + it('publishes at most 12 shots, whatever the limit asks', () => { + const changes = Array.from({length: 30}, (_, i) => ({ + key: `s${i}`, + diffPixels: i, + })); + expect(framesToPublish({changes}, 100)).toHaveLength(MAX_SHOTS); + expect(MAX_SHOTS).toBe(12); + }); + + it('writes only assets/pr-', () => { + expect(assetsBranch(42)).toBe('assets/pr-42'); + for (const bad of [0, -1, 1.5, NaN, '42/../main']) + expect(() => assetsBranch(bad)).toThrow(); + }); +}); + +describe('publishFrames', () => { + it("commits the frames on top of the PR's existing assets branch", async () => { + const {calls, request} = fakeGitHub({branchExists: true}); + const base = await publishFrames({ + input: input({changes: [{key: 'b', diffPixels: 2}]}), + pr: 9171, + headSha: HEAD, + repository: 'meta/astryx', + limit: 12, + request, + }); + expect(base).toEqual({ + imageBase: `https://raw.githubusercontent.com/meta/astryx/assets/pr-9171/visual/${HEAD}/`, + published: ['before/b.png', 'after/b.png', 'diff/b.png'], + }); + const tree = calls.find(call => call.route.endsWith('/git/trees')).body; + expect(tree.base_tree).toBe('parent-tree'); + expect(tree.tree.map(entry => entry.path)).not.toContain(MARKER); + expect(tree.tree.map(entry => entry.path)).toEqual([ + `visual/${HEAD}/before/b.png`, + `visual/${HEAD}/after/b.png`, + `visual/${HEAD}/diff/b.png`, + ]); + expect( + calls.find(call => call.route.endsWith('/git/commits')).body.parents, + ).toEqual(['parent']); + expect(calls.at(-1)).toMatchObject({ + method: 'PATCH', + route: '/repos/meta/astryx/git/refs/heads/assets/pr-9171', + body: {sha: 'commit'}, + }); + }); + + it('skips a frame that is not an image, and touches no other ref', async () => { + const root = input({changes: [{key: 'a', diffPixels: 1}]}); + fs.writeFileSync(path.join(root, 'diff', 'a.png'), ''); + const {calls, request} = fakeGitHub({branchExists: true}); + await publishFrames({ + input: root, + pr: 7, + headSha: HEAD, + repository: 'meta/astryx', + limit: 12, + request, + }); + const tree = calls.find(call => call.route.endsWith('/git/trees')).body; + expect(tree.tree.map(entry => entry.path)).toEqual([ + `visual/${HEAD}/before/a.png`, + `visual/${HEAD}/after/a.png`, + ]); + const refs = calls.filter(call => call.route.includes('/git/ref')); + expect( + refs.every( + call => + call.route.includes('assets/pr-7') || + call.route.endsWith('/git/refs'), + ), + ).toBe(true); + }); + + it('creates the branch when it does not exist, and writes nothing without changes', async () => { + const created = fakeGitHub({branchExists: false}); + await publishFrames({ + input: input({added: ['added']}), + pr: 5, + headSha: HEAD, + repository: 'meta/astryx', + limit: 12, + request: created.request, + }); + // Only a branch this script creates carries the pruning marker. + const createdTree = created.calls.find(call => + call.route.endsWith('/git/trees'), + ).body; + expect(createdTree.tree.map(entry => entry.path)).toContain(MARKER); + expect(created.calls.at(-1)).toMatchObject({ + method: 'POST', + route: '/repos/meta/astryx/git/refs', + body: {ref: 'refs/heads/assets/pr-5'}, + }); + const none = fakeGitHub({branchExists: true}); + await expect( + publishFrames({ + input: input({changes: []}), + pr: 5, + headSha: HEAD, + repository: 'meta/astryx', + limit: 12, + request: none.request, + }), + ).resolves.toBeNull(); + expect(none.calls).toEqual([]); + }); +}); diff --git a/.github/scripts/visual-gate/workflow-concurrency.test.mjs b/.github/scripts/visual-gate/workflow-concurrency.test.mjs index 35ca0688e2d95..a83fb51cd49e8 100644 --- a/.github/scripts/visual-gate/workflow-concurrency.test.mjs +++ b/.github/scripts/visual-gate/workflow-concurrency.test.mjs @@ -63,7 +63,10 @@ describe('PR report and deployment workflow contracts', () => { ); expect(value.jobs[job].concurrency['cancel-in-progress']).toBe(false); } - expect(value.jobs.comment.needs).toBe('resolve'); + expect(value.jobs.comment.needs).toEqual(['resolve', 'frames']); + expect(value.jobs.frames.concurrency.group).toBe( + 'pr-frames-${{ needs.resolve.outputs.pr_number }}', + ); // The comment job reads this head's preview state once; no job runs on a // deployment event, and none waits for one. expect(value.jobs.comment.permissions.deployments).toBe('read'); @@ -78,7 +81,7 @@ describe('PR report and deployment workflow contracts', () => { const value = workflow('pr-comment.yml'); const reconcile = value.slice( value.indexOf(' spec-only-reconcile:'), - value.indexOf(' comment:'), + value.indexOf(' frames:'), ); expect(reconcile).toContain('reconcilePrComment'); expect(reconcile).toContain('createIfMissing: false'); diff --git a/.github/workflows/pr-comment.yml b/.github/workflows/pr-comment.yml index d16bcf1534527..34ed7d92f6c5f 100644 --- a/.github/workflows/pr-comment.yml +++ b/.github/workflows/pr-comment.yml @@ -183,8 +183,96 @@ jobs: 'The current change has no visual or preview-producing scope. Visual checks are not applicable; preview links are not shown.', }); - comment: + # The changed visual frames behind the report's thumbnails, on the pull + # request's assets branch. The only job here that writes repository + # contents: it runs trusted code from main (never pull-request code), keeps + # no credentials in its checkout, and publish-frames.mjs writes only + # assets/pr-, and only validated PNG/WebP frames. + frames: needs: resolve + concurrency: + group: pr-frames-${{ needs.resolve.outputs.pr_number }} + cancel-in-progress: false + if: >- + always() && + github.event.action == 'completed' && + needs.resolve.outputs.valid == 'true' && + needs.resolve.outputs.spec_only != 'true' && + needs.resolve.outputs.tooling_only != 'true' + runs-on: ubuntu-24.04-arm + permissions: + actions: read + contents: write + outputs: + image_base: ${{ steps.frames.outputs.image_base }} + published: ${{ steps.frames.outputs.published }} + failed: >- + ${{ steps.checkout.outcome == 'failure' || + steps.setup.outcome == 'failure' || + steps.visual.outcome == 'failure' || + steps.frames.outcome == 'failure' }} + steps: + - name: Checkout trusted default-branch code + id: checkout + continue-on-error: true + uses: actions/checkout@v7 + with: + ref: main + persist-credentials: false + + - name: Setup Node and pnpm + id: setup + continue-on-error: true + if: steps.checkout.outcome == 'success' + uses: ./.github/actions/setup + + - name: Download canonical visual report + id: visual-download + if: steps.setup.outcome == 'success' + uses: actions/download-artifact@v8 + with: + name: visual-pr-report + path: visual-pr-report + run-id: ${{ needs.resolve.outputs.run_id }} + github-token: ${{ secrets.GITHUB_TOKEN }} + continue-on-error: true + + # The same re-encoding the comment job applies: only JSON and + # re-encoded PNGs from the exact CI attempt. + - name: Prepare canonical visual report + id: visual + continue-on-error: true + if: steps.visual-download.outcome == 'success' + env: + HEAD_SHA: ${{ needs.resolve.outputs.head_sha }} + RUN_ID: ${{ needs.resolve.outputs.run_id }} + RUN_ATTEMPT: ${{ needs.resolve.outputs.run_attempt }} + run: | + node .github/scripts/visual-gate/publish-pr-report.mjs \ + --input visual-pr-report \ + --output trusted-visual \ + --head-sha "$HEAD_SHA" \ + --run-id "$RUN_ID" \ + --run-attempt "$RUN_ATTEMPT" + + - name: Publish changed visual frames + id: frames + continue-on-error: true + if: steps.visual.outcome == 'success' + env: + GITHUB_TOKEN: ${{ github.token }} + PR_NUMBER: ${{ needs.resolve.outputs.pr_number }} + HEAD_SHA: ${{ needs.resolve.outputs.head_sha }} + run: | + node .github/scripts/visual-gate/publish-frames.mjs \ + --input trusted-visual \ + --pr "$PR_NUMBER" \ + --head-sha "$HEAD_SHA" \ + --repository "$GITHUB_REPOSITORY" \ + --limit 12 + + comment: + needs: [resolve, frames] concurrency: group: pr-report-${{ needs.resolve.outputs.pr_number }} cancel-in-progress: false @@ -385,6 +473,8 @@ jobs: RUN_ID: ${{ steps.identity.outputs.run_id }} RUN_ATTEMPT: ${{ steps.identity.outputs.run_attempt }} SOURCE_CONCLUSION: ${{ steps.identity.outputs.source_conclusion }} + VISUAL_IMAGE_BASE: ${{ needs.frames.outputs.image_base }} + VISUAL_PUBLISHED: ${{ needs.frames.outputs.published }} with: retries: 3 script: | @@ -408,6 +498,10 @@ jobs: sourceRunAttempt: Number(process.env.RUN_ATTEMPT), sourceConclusion: process.env.SOURCE_CONCLUSION, }, + visualFrames: { + imageBase: process.env.VISUAL_IMAGE_BASE, + published: process.env.VISUAL_PUBLISHED, + }, }); # The CI-completion lane runs on workflow_run, so GitHub attaches its jobs to @@ -416,7 +510,7 @@ jobs: # continues on error, and this job reports their outcome as the PR Comment # status on the pull request's own head. report: - needs: [resolve, spec-only-reconcile, comment] + needs: [resolve, spec-only-reconcile, frames, comment] if: >- always() && github.event_name == 'workflow_run' && @@ -437,6 +531,7 @@ jobs: FAILED: >- ${{ needs.resolve.outputs.failed == 'true' || needs.spec-only-reconcile.outputs.failed == 'true' || + needs.frames.outputs.failed == 'true' || needs.comment.outputs.failed == 'true' || contains(needs.*.result, 'failure') || contains(needs.*.result, 'cancelled') }} diff --git a/scripts/prune-branches.sh b/scripts/prune-branches.sh index f5ad24b161c67..b20ebdc594b31 100755 --- a/scripts/prune-branches.sh +++ b/scripts/prune-branches.sh @@ -2,6 +2,7 @@ # Copyright (c) Meta Platforms, Inc. and affiliates. # scripts/prune-branches.sh — Delete remote branches with merged/closed PRs +# (an `assets/pr-` branch the PR report created goes with PR #) # # Usage: # ./scripts/prune-branches.sh # dry run (default) @@ -90,7 +91,15 @@ stale_cutoff="$3" repo="$4" safe_name=$(echo "$branch" | tr '/' '_') -state=$(gh pr list --repo "$repo" --head "$branch" --state all --json state --jq '.[0].state // "none"' 2>/dev/null || echo "error") +# An `assets/pr-` branch the PR report created (it carries the +# `.pr-report-frames` marker) goes with PR #. Every other branch, including +# assets branches pushed by hand, keeps the rule below. +if [[ "$branch" =~ ^assets/pr-([0-9]+)$ ]] && git cat-file -e "origin/$branch:.pr-report-frames" 2>/dev/null; then + state=$(gh pr view "${BASH_REMATCH[1]}" --repo "$repo" --json state --jq '.state' 2>/dev/null || echo "error") + [ "$state" = "error" ] && state="OPEN" +else + state=$(gh pr list --repo "$repo" --head "$branch" --state all --json state --jq '.[0].state // "none"' 2>/dev/null || echo "error") +fi if [ "$state" = "MERGED" ] || [ "$state" = "CLOSED" ]; then echo "$branch" > "$tmp_dir/delete/$safe_name" diff --git a/scripts/prune-branches.test.mjs b/scripts/prune-branches.test.mjs new file mode 100644 index 0000000000000..d621e20a95127 --- /dev/null +++ b/scripts/prune-branches.test.mjs @@ -0,0 +1,79 @@ +// Copyright (c) Meta Platforms, Inc. and affiliates. + +/** + * @file prune-branches.test.mjs + * Pins that the daily branch pruner deletes an `assets/pr-` branch the PR + * report created (it carries `.pr-report-frames`) once PR # is merged or + * closed, keeps it while the PR is open, and does not delete an assets branch + * without the marker, such as screenshots pushed by hand, for its PR closing. + */ + +import {execFileSync} from 'node:child_process'; +import fs from 'node:fs'; +import os from 'node:os'; +import path from 'node:path'; +import {fileURLToPath} from 'node:url'; +import {afterEach, describe, expect, it} from 'vitest'; + +const SCRIPT = path.join( + path.dirname(fileURLToPath(import.meta.url)), + 'prune-branches.sh', +); +const roots = []; +afterEach(() => { + for (const root of roots.splice(0)) + fs.rmSync(root, {recursive: true, force: true}); +}); + +function setup(prStates) { + const root = fs.mkdtempSync(path.join(os.tmpdir(), 'prune-')); + roots.push(root); + const git = (cwd, ...args) => + execFileSync('git', args, {cwd, encoding: 'utf8'}); + const origin = path.join(root, 'origin.git'); + const work = path.join(root, 'work'); + git(root, 'init', '-q', '--bare', origin); + git(root, 'init', '-q', '-b', 'main', work); + git(work, 'config', 'user.email', 'test@test.co'); + git(work, 'config', 'user.name', 'Test'); + git(work, 'commit', '-q', '--allow-empty', '-m', 'base'); + git(work, 'remote', 'add', 'origin', origin); + // assets/pr-3 is pushed by hand: no marker. + git(work, 'push', '-q', 'origin', 'main', 'main:assets/pr-3'); + fs.writeFileSync(path.join(work, '.pr-report-frames'), 'marker\n'); + git(work, 'add', '.pr-report-frames'); + git(work, 'commit', '-q', '-m', 'frames'); + git(work, 'push', '-q', 'origin', 'HEAD:assets/pr-1', 'HEAD:assets/pr-2'); + // A stand-in for gh: `pr view ` answers from prStates; `pr list` finds no PR. + const bin = path.join(root, 'bin'); + fs.mkdirSync(bin); + fs.writeFileSync( + path.join(bin, 'gh'), + `#!/usr/bin/env bash\nif [ "$1 $2" = "pr view" ]; then\n case "$3" in\n${Object.entries( + prStates, + ) + .map(([n, state]) => ` ${n}) echo ${state} ;;`) + .join('\n')}\n esac\nelse\n echo none\nfi\n`, + {mode: 0o755}, + ); + return {work, bin}; +} + +describe('prune-branches.sh', () => { + it("deletes a closed PR's marked assets branch, keeps an open PR's, and never an unmarked one", () => { + const {work, bin} = setup({1: 'MERGED', 2: 'OPEN', 3: 'CLOSED'}); + const out = execFileSync('bash', [SCRIPT], { + cwd: work, + encoding: 'utf8', + env: {...process.env, PATH: `${bin}:${process.env.PATH}`}, + }).replace(/\u001b\[[0-9;]*m/g, ''); + const willDelete = out.slice( + out.indexOf('Merged/closed PR (will delete):'), + ); + expect(willDelete).toContain('assets/pr-1'); + expect(willDelete).not.toContain('assets/pr-2'); + // assets/pr-3 has no marker: its PR is closed, yet it is not deleted. + expect(out).not.toMatch(/^\s+assets\/pr-3$/m); + expect(out).toContain('0 branches (no PR'); + }); +});