From 125dcd01748ef381631b6df091aaa20f4189a8a1 Mon Sep 17 00:00:00 2001 From: Marc Chan Date: Tue, 26 May 2026 14:17:04 +0800 Subject: [PATCH] fix(ci): run fork visual reports from trusted code (#2935) * fix: run fork visual reports from trusted code * fix: auto-approve strict web visual capture * fix: address visual report review feedback Generated-By: looper 0.9.1 (runner=fixer, agent=opencode) * fix: propagate visual report storage failures Generated-By: looper 0.9.1 (runner=fixer, agent=opencode) * fix: validate PR screenshots before upload Generated-By: looper 0.9.1 (runner=fixer, agent=opencode) * fix: validate visual PR identity before comment * fix: harden fork visual report validation Generated-By: looper 0.9.1 (runner=fixer, agent=opencode) * fix: address remaining fork visual report review feedback Generated-By: looper 0.9.1 (runner=fixer, agent=opencode) * fix: handle stale fork visual report lookup Generated-By: looper 0.9.1 (runner=fixer, agent=opencode) * fix: allow stale fork visual report fallback Generated-By: looper 0.9.1 (runner=fixer, agent=opencode) --- .../workflows/fork-pr-workflow-approval.yml | 3 +- .github/workflows/visual-pr-comment.yml | 267 +++++++++++------ e2e/scripts/visual-report.ts | 101 +++++-- e2e/tests/visual-report.test.ts | 273 ++++++++++++++++++ scripts/approve-fork-pr-workflows.test.ts | 160 ++++++++++ scripts/approve-fork-pr-workflows.ts | 30 +- 6 files changed, 720 insertions(+), 114 deletions(-) diff --git a/.github/workflows/fork-pr-workflow-approval.yml b/.github/workflows/fork-pr-workflow-approval.yml index a75741ffe..3f6b13201 100644 --- a/.github/workflows/fork-pr-workflow-approval.yml +++ b/.github/workflows/fork-pr-workflow-approval.yml @@ -4,7 +4,8 @@ name: fork-pr-workflow-approval # or executes the fork head; the TypeScript policy script only reads PR metadata # through the GitHub API and approves pending pull_request runs when the touched # paths are inside the low-risk source allowlist. It only approves low-privilege -# pull_request workflows (`ci`, visual capture, visual verify); privileged +# pull_request workflows (`ci`, visual verify, and strict web-source visual +# capture); privileged # workflow_run / release / deploy workflows stay on manual gates. on: pull_request_target: diff --git a/.github/workflows/visual-pr-comment.yml b/.github/workflows/visual-pr-comment.yml index f12058e5f..8c6953fca 100644 --- a/.github/workflows/visual-pr-comment.yml +++ b/.github/workflows/visual-pr-comment.yml @@ -27,20 +27,21 @@ concurrency: jobs: comment: name: Publish PR visual diff comment - # Fork PR capture artifacts are untrusted. Publish comments/R2 reports for - # same-repository PRs automatically; fork captures require maintainer - # workflow_dispatch with a specific capture run id and PR number. - if: ${{ github.event_name == 'workflow_dispatch' || (github.event.workflow_run.conclusion == 'success' && github.event.workflow_run.head_repository.full_name == github.repository) }} + # Fork PR capture artifacts are untrusted. Always validate the live PR state + # and only execute trusted base-repository code before publishing comments + # or reports. + if: ${{ github.event_name == 'workflow_dispatch' || github.event.workflow_run.conclusion == 'success' }} runs-on: ubuntu-24.04 timeout-minutes: 15 steps: - - name: Checkout + - name: Checkout same-repository workflow head + if: ${{ github.event_name == 'workflow_run' && github.event.workflow_run.head_repository.full_name == github.repository }} uses: actions/checkout@v6.0.2 with: fetch-depth: 0 - repository: ${{ github.event_name == 'workflow_run' && github.event.workflow_run.head_repository.full_name || github.repository }} - ref: ${{ github.event_name == 'workflow_run' && github.event.workflow_run.head_sha || github.sha }} + repository: ${{ github.event.workflow_run.head_repository.full_name }} + ref: ${{ github.event.workflow_run.head_sha }} - name: Setup pnpm uses: pnpm/action-setup@v6.0.8 @@ -57,7 +58,7 @@ jobs: uses: actions/download-artifact@v8 with: run-id: ${{ github.event.workflow_run.id || inputs.capture_run_id }} - path: visual-artifact + path: ${{ runner.temp }}/visual-artifact merge-multiple: true github-token: ${{ github.token }} @@ -68,16 +69,19 @@ jobs: GH_TOKEN: ${{ github.token }} WORKFLOW_PR_NUMBER: ${{ github.event.workflow_run.pull_requests[0].number || inputs.pr_number }} WORKFLOW_HEAD_SHA: ${{ github.event.workflow_run.head_sha || '' }} + WORKFLOW_HEAD_REPOSITORY: ${{ github.event.workflow_run.head_repository.full_name || '' }} + WORKFLOW_HEAD_BRANCH: ${{ github.event.workflow_run.head_branch || '' }} WORKFLOW_RUN_ID: ${{ github.event.workflow_run.id || inputs.capture_run_id }} run: | set -euo pipefail - manifest="visual-artifact/visual-report/manifest.json" + artifact_dir="$RUNNER_TEMP/visual-artifact" + manifest="$artifact_dir/visual-report/manifest.json" if [ ! -f "$manifest" ]; then - manifest="visual-artifact/manifest.json" + manifest="$artifact_dir/manifest.json" fi if [ ! -f "$manifest" ]; then echo "Capture manifest not found" >&2 - find visual-artifact -maxdepth 4 -type f >&2 + find "$artifact_dir" -maxdepth 4 -type f >&2 exit 1 fi manifest_pr_number="$(jq -r '.pr_number' "$manifest")" @@ -88,22 +92,6 @@ jobs: echo "Invalid manifest pr_number: $manifest_pr_number" >&2 exit 1 fi - derived_pr_number="$WORKFLOW_PR_NUMBER" - if [ -z "$derived_pr_number" ] && [ -n "$WORKFLOW_HEAD_SHA" ]; then - derived_pr_number="$(gh api "repos/$GITHUB_REPOSITORY/commits/$WORKFLOW_HEAD_SHA/pulls" --jq '.[0].number // empty')" - fi - pr_number="$derived_pr_number" - if [ -z "$pr_number" ]; then - pr_number="$manifest_pr_number" - fi - if ! [[ "$pr_number" =~ ^[0-9]+$ ]]; then - echo "Unable to derive PR number from workflow event, capture manifest, or GitHub API for workflow head $WORKFLOW_HEAD_SHA." >&2 - exit 1 - fi - if [ -n "$derived_pr_number" ] && [ "$manifest_pr_number" != "$derived_pr_number" ]; then - echo "Manifest pr_number ($manifest_pr_number) does not match GitHub-derived PR number ($derived_pr_number)." >&2 - exit 1 - fi if ! [[ "$base_sha" =~ ^[0-9a-f]{40}$ ]]; then echo "Invalid manifest base_sha: $base_sha" >&2 exit 1 @@ -116,13 +104,73 @@ jobs: echo "Invalid manifest capture_outcome: $capture_outcome" >&2 exit 1 fi + source_head_sha="$WORKFLOW_HEAD_SHA" + source_head_repository="$WORKFLOW_HEAD_REPOSITORY" + source_head_branch="$WORKFLOW_HEAD_BRANCH" + if [ -z "$source_head_sha" ] || [ -z "$source_head_repository" ] || [ -z "$source_head_branch" ]; then + run_json="$(gh api "repos/$GITHUB_REPOSITORY/actions/runs/$WORKFLOW_RUN_ID")" + if [ -z "$source_head_sha" ]; then + source_head_sha="$(jq -r '.head_sha // empty' <<< "$run_json")" + fi + if [ -z "$source_head_repository" ]; then + source_head_repository="$(jq -r '.head_repository.full_name // empty' <<< "$run_json")" + fi + if [ -z "$source_head_branch" ]; then + source_head_branch="$(jq -r '.head_branch // empty' <<< "$run_json")" + fi + fi manifest_head="$(jq -r '.head_sha' "$manifest")" if ! [[ "$manifest_head" =~ ^[0-9a-f]{40}$ ]]; then echo "Invalid manifest head_sha: $manifest_head" >&2 exit 1 fi - if [ -n "$WORKFLOW_HEAD_SHA" ] && [ "$manifest_head" != "$WORKFLOW_HEAD_SHA" ]; then - echo "Artifact manifest head_sha ($manifest_head) does not match workflow_run head_sha ($WORKFLOW_HEAD_SHA)." >&2 + if ! [[ "$source_head_sha" =~ ^[0-9a-f]{40}$ ]]; then + echo "Unable to resolve trusted workflow_run head_sha for run $WORKFLOW_RUN_ID." >&2 + exit 1 + fi + if [ "$manifest_head" != "$source_head_sha" ]; then + echo "Artifact manifest head_sha ($manifest_head) does not match workflow_run head_sha ($source_head_sha)." >&2 + exit 1 + fi + if [ -z "$source_head_repository" ]; then + echo "Unable to resolve trusted workflow_run head repository for run $WORKFLOW_RUN_ID." >&2 + exit 1 + fi + if [ -z "$source_head_branch" ]; then + echo "Unable to resolve trusted workflow_run head branch for run $WORKFLOW_RUN_ID." >&2 + exit 1 + fi + pr_number="$WORKFLOW_PR_NUMBER" + if [ -z "$pr_number" ]; then + head_owner="${source_head_repository%%/*}" + if [ -z "$head_owner" ] || [ "$head_owner" = "$source_head_repository" ]; then + echo "Unable to resolve trusted workflow_run head owner from $source_head_repository." >&2 + exit 1 + fi + encoded_head="$(jq -rn --arg head "$head_owner:$source_head_branch" '$head|@uri')" + matching_pr_numbers="$( + gh api "repos/$GITHUB_REPOSITORY/pulls?state=open&head=$encoded_head&per_page=100" \ + | jq -r --arg repo "$source_head_repository" \ + '.[] | select((.head.repo.full_name // "") == $repo) | .number' \ + | paste -sd ' ' - + )" + match_count="$(wc -w <<< "$matching_pr_numbers" | tr -d ' ')" + if [ "$match_count" -gt 1 ]; then + echo "Unable to resolve a unique open PR from trusted workflow metadata for $source_head_repository@$source_head_branch ($source_head_sha); found $match_count matches." >&2 + exit 1 + fi + if [ "$match_count" -eq 1 ]; then + pr_number="$matching_pr_numbers" + else + pr_number="$manifest_pr_number" + fi + fi + if ! [[ "$pr_number" =~ ^[0-9]+$ ]]; then + echo "Unable to derive PR number from trusted workflow metadata for workflow head $source_head_sha." >&2 + exit 1 + fi + if [ -n "$WORKFLOW_PR_NUMBER" ] && [ "$manifest_pr_number" != "$WORKFLOW_PR_NUMBER" ]; then + echo "Manifest pr_number ($manifest_pr_number) does not match workflow_run PR number ($WORKFLOW_PR_NUMBER)." >&2 exit 1 fi { @@ -132,12 +180,96 @@ jobs: echo "base_sha=$base_sha" echo "run_id=$run_id" echo "capture_outcome=$capture_outcome" + echo "source_head_repository=$source_head_repository" + echo "source_head_branch=$source_head_branch" } >> "$GITHUB_OUTPUT" - name: Validate live PR state for trusted checkout - if: ${{ github.event_name == 'workflow_dispatch' }} id: trusted-pr shell: bash + env: + GH_TOKEN: ${{ github.token }} + EVENT_NAME: ${{ github.event_name }} + PR_NUMBER: ${{ steps.manifest.outputs.pr_number }} + ARTIFACT_HEAD_SHA: ${{ steps.manifest.outputs.head_sha }} + ARTIFACT_BASE_SHA: ${{ steps.manifest.outputs.base_sha }} + SOURCE_HEAD_REPOSITORY: ${{ steps.manifest.outputs.source_head_repository }} + SOURCE_HEAD_BRANCH: ${{ steps.manifest.outputs.source_head_branch }} + run: | + set -euo pipefail + skip_or_fail_stale() { + local message="$1" + if [ "$EVENT_NAME" = "workflow_dispatch" ]; then + echo "$message" >&2 + exit 1 + fi + echo "$message" + echo "stale=true" >> "$GITHUB_OUTPUT" + exit 0 + } + + pr_json="$(gh api "repos/$GITHUB_REPOSITORY/pulls/$PR_NUMBER")" + pr_state="$(jq -r '.state' <<< "$pr_json")" + pr_is_draft="$(jq -r '.draft' <<< "$pr_json")" + pr_is_cross_repository="$(jq -r '(.head.repo.full_name // "") != (.base.repo.full_name // "")' <<< "$pr_json")" + current_head="$(jq -r '.head.sha' <<< "$pr_json")" + current_base="$(jq -r '.base.sha' <<< "$pr_json")" + current_head_repository="$(jq -r '.head.repo.full_name // empty' <<< "$pr_json")" + current_head_branch="$(jq -r '.head.ref // empty' <<< "$pr_json")" + if [ "$pr_state" != "open" ]; then + skip_or_fail_stale "Skipping trusted checkout for PR $PR_NUMBER because state is $pr_state." + fi + if [ "$pr_is_draft" != "false" ]; then + skip_or_fail_stale "Skipping trusted checkout for PR $PR_NUMBER because it is draft." + fi + if [ "$current_head" != "$ARTIFACT_HEAD_SHA" ]; then + skip_or_fail_stale "Skipping stale visual artifact for $ARTIFACT_HEAD_SHA; current PR head is $current_head." + fi + if [ "$current_base" != "$ARTIFACT_BASE_SHA" ]; then + skip_or_fail_stale "Skipping stale visual artifact for base $ARTIFACT_BASE_SHA; current PR base is $current_base." + fi + if [ "$current_head_repository" != "$SOURCE_HEAD_REPOSITORY" ]; then + echo "Artifact PR head repository ($current_head_repository) does not match trusted workflow_run head repository ($SOURCE_HEAD_REPOSITORY)." >&2 + exit 1 + fi + if [ "$current_head_branch" != "$SOURCE_HEAD_BRANCH" ]; then + echo "Artifact PR head branch ($current_head_branch) does not match trusted workflow_run head branch ($SOURCE_HEAD_BRANCH)." >&2 + exit 1 + fi + { + echo "stale=false" + echo "base_sha=$current_base" + echo "is_cross_repository=$pr_is_cross_repository" + } >> "$GITHUB_OUTPUT" + + - name: Checkout trusted base revision + if: ${{ steps.trusted-pr.outputs.stale != 'true' && (github.event_name == 'workflow_dispatch' || github.event.workflow_run.head_repository.full_name != github.repository) }} + uses: actions/checkout@v6.0.2 + with: + fetch-depth: 0 + repository: ${{ github.repository }} + ref: ${{ steps.trusted-pr.outputs.base_sha }} + + - name: Resolve pnpm store path + if: ${{ steps.trusted-pr.outputs.stale != 'true' }} + id: pnpm-store + shell: bash + run: echo "path=$(pnpm store path --silent)" >> "$GITHUB_OUTPUT" + + - name: Restore pnpm store cache + if: ${{ steps.trusted-pr.outputs.stale != 'true' }} + uses: actions/cache/restore@v5.0.5 + with: + path: ${{ steps.pnpm-store.outputs.path }} + key: pnpm-store-${{ runner.os }}-${{ hashFiles('pnpm-lock.yaml') }} + + - name: Install dependencies + if: ${{ steps.trusted-pr.outputs.stale != 'true' }} + run: pnpm install --frozen-lockfile + + - name: Stop stale visual runs + if: ${{ steps.trusted-pr.outputs.stale != 'true' }} + id: stale env: GH_TOKEN: ${{ github.token }} PR_NUMBER: ${{ steps.manifest.outputs.pr_number }} @@ -151,58 +283,15 @@ jobs: current_head="$(jq -r '.headRefOid' <<< "$pr_json")" current_base="$(jq -r '.baseRefOid' <<< "$pr_json")" if [ "$pr_state" != "OPEN" ]; then - echo "Refusing trusted checkout for PR $PR_NUMBER because state is $pr_state." >&2 - exit 1 + echo "Skipping visual report for PR $PR_NUMBER because state is $pr_state." + echo "stale=true" >> "$GITHUB_OUTPUT" + exit 0 fi if [ "$pr_is_draft" != "false" ]; then - echo "Refusing trusted checkout for PR $PR_NUMBER because it is draft." >&2 - exit 1 + echo "Skipping visual report for PR $PR_NUMBER because it is draft." + echo "stale=true" >> "$GITHUB_OUTPUT" + exit 0 fi - if [ "$current_head" != "$ARTIFACT_HEAD_SHA" ]; then - echo "Artifact head_sha ($ARTIFACT_HEAD_SHA) does not match live PR head ($current_head)." >&2 - exit 1 - fi - if [ "$current_base" != "$ARTIFACT_BASE_SHA" ]; then - echo "Artifact base_sha ($ARTIFACT_BASE_SHA) does not match live PR base ($current_base)." >&2 - exit 1 - fi - echo "base_sha=$current_base" >> "$GITHUB_OUTPUT" - - - name: Checkout trusted base revision - if: ${{ github.event_name == 'workflow_dispatch' }} - uses: actions/checkout@v6.0.2 - with: - clean: false - fetch-depth: 0 - repository: ${{ github.repository }} - ref: ${{ steps.trusted-pr.outputs.base_sha }} - - - name: Resolve pnpm store path - id: pnpm-store - shell: bash - run: echo "path=$(pnpm store path --silent)" >> "$GITHUB_OUTPUT" - - - name: Restore pnpm store cache - uses: actions/cache/restore@v5.0.5 - with: - path: ${{ steps.pnpm-store.outputs.path }} - key: pnpm-store-${{ runner.os }}-${{ hashFiles('pnpm-lock.yaml') }} - - - name: Install dependencies - run: pnpm install --frozen-lockfile - - - name: Stop stale visual runs - id: stale - env: - GH_TOKEN: ${{ github.token }} - PR_NUMBER: ${{ steps.manifest.outputs.pr_number }} - ARTIFACT_HEAD_SHA: ${{ steps.manifest.outputs.head_sha }} - ARTIFACT_BASE_SHA: ${{ steps.manifest.outputs.base_sha }} - run: | - set -euo pipefail - pr_json="$(gh pr view "$PR_NUMBER" --repo "$GITHUB_REPOSITORY" --json headRefOid,baseRefOid)" - current_head="$(jq -r '.headRefOid' <<< "$pr_json")" - current_base="$(jq -r '.baseRefOid' <<< "$pr_json")" if [ "$current_head" != "$ARTIFACT_HEAD_SHA" ]; then echo "Skipping stale visual artifact for $ARTIFACT_HEAD_SHA; current PR head is $current_head." echo "stale=true" >> "$GITHUB_OUTPUT" @@ -216,7 +305,7 @@ jobs: echo "stale=false" >> "$GITHUB_OUTPUT" - name: Build visual diff report - if: ${{ steps.stale.outputs.stale != 'true' }} + if: ${{ steps.trusted-pr.outputs.stale != 'true' && steps.stale.outputs.stale != 'true' }} env: R2_ACCOUNT_ID: ${{ secrets.R2_ACCOUNT_ID }} R2_ACCESS_KEY_ID: ${{ secrets.R2_ACCESS_KEY_ID }} @@ -235,12 +324,12 @@ jobs: --head-sha "${{ steps.manifest.outputs.head_sha }}" \ --base-sha "${{ steps.manifest.outputs.base_sha }}" \ --capture-outcome "${{ steps.manifest.outputs.capture_outcome }}" \ - --screenshots ../visual-artifact/visual-screenshots \ + --screenshots "$RUNNER_TEMP/visual-artifact/visual-screenshots" \ --comment-out ui/reports/visual-report/comment.md \ --manifest-out ui/reports/visual-report/report-manifest.json - name: Upsert PR comment - if: ${{ steps.stale.outputs.stale != 'true' }} + if: ${{ steps.trusted-pr.outputs.stale != 'true' && steps.stale.outputs.stale != 'true' }} env: GH_TOKEN: ${{ github.token }} PR_NUMBER: ${{ steps.manifest.outputs.pr_number }} @@ -248,9 +337,19 @@ jobs: ARTIFACT_BASE_SHA: ${{ steps.manifest.outputs.base_sha }} run: | set -euo pipefail - pr_json="$(gh pr view "$PR_NUMBER" --repo "$GITHUB_REPOSITORY" --json headRefOid,baseRefOid)" + pr_json="$(gh pr view "$PR_NUMBER" --repo "$GITHUB_REPOSITORY" --json state,isDraft,headRefOid,baseRefOid)" + pr_state="$(jq -r '.state' <<< "$pr_json")" + pr_is_draft="$(jq -r '.isDraft' <<< "$pr_json")" current_head="$(jq -r '.headRefOid' <<< "$pr_json")" current_base="$(jq -r '.baseRefOid' <<< "$pr_json")" + if [ "$pr_state" != "OPEN" ]; then + echo "Skipping visual comment for PR $PR_NUMBER because state is $pr_state." + exit 0 + fi + if [ "$pr_is_draft" != "false" ]; then + echo "Skipping visual comment for PR $PR_NUMBER because it is draft." + exit 0 + fi if [ "$current_head" != "$ARTIFACT_HEAD_SHA" ]; then echo "Skipping stale visual comment for $ARTIFACT_HEAD_SHA; current PR head is $current_head." exit 0 @@ -270,7 +369,7 @@ jobs: fi - name: Upload visual report artifact - if: ${{ always() && steps.stale.outputs.stale != 'true' }} + if: ${{ always() && steps.trusted-pr.outputs.stale != 'true' && steps.stale.outputs.stale != 'true' }} uses: actions/upload-artifact@v7 with: name: visual-pr-report-${{ steps.manifest.outputs.pr_number }}-${{ steps.manifest.outputs.run_id }} diff --git a/e2e/scripts/visual-report.ts b/e2e/scripts/visual-report.ts index 383454fa3..a5e12ff8d 100644 --- a/e2e/scripts/visual-report.ts +++ b/e2e/scripts/visual-report.ts @@ -54,6 +54,13 @@ type ComparedCase = { error?: string; }; +type CompareCaseOps = { + putFile?: (r2: R2Config, key: string, filePath: string) => Promise; + findBaseline?: (r2: R2Config, caseName: string, candidateShas: string[]) => Promise; + downloadObject?: (r2: R2Config, key: string, outputPath: string) => Promise; + writeDiffPng?: (mainPath: string, prPath: string, diffPath: string) => Promise; +}; + const scriptDir = path.dirname(fileURLToPath(import.meta.url)); const e2eDir = path.resolve(scriptDir, '..'); @@ -154,7 +161,7 @@ async function comparePr(options: ParsedArgs): Promise { console.log(`Wrote visual report for ${compared.length} cases to ${commentOut}.`); } -async function compareCase(input: { +export async function compareCase(input: { r2: R2Config; prNumber: string; runId: string; @@ -162,12 +169,29 @@ async function compareCase(input: { visualCase: VisualCase; candidateShas: string[]; outputDir: string; -}): Promise { +}, ops: CompareCaseOps = {}): Promise { const { r2, prNumber, runId, headSha, visualCase, candidateShas, outputDir } = input; + const putFileOp = ops.putFile ?? putFile; + const findBaselineOp = ops.findBaseline ?? findBaseline; + const downloadObjectOp = ops.downloadObject ?? downloadObject; + const writeDiffPngOp = ops.writeDiffPng ?? writeDiffPng; const prKey = prImageKey(prNumber, runId, 'pr', visualCase.name); - await putFile(r2, prKey, visualCase.path); - const baseline = await findBaseline(r2, visualCase.name, candidateShas); + const visualBuffer = await readFile(visualCase.path); + + try { + validatePngBuffer(visualBuffer, visualCase.path); + } catch (error) { + return { + name: visualCase.name, + status: 'failed', + error: error instanceof Error ? error.message : String(error), + }; + } + + await putFileOp(r2, prKey, visualCase.path); + + const baseline = await findBaselineOp(r2, visualCase.name, candidateShas); if (baseline == null) { return { name: visualCase.name, @@ -178,36 +202,42 @@ async function compareCase(input: { const mainPath = path.join(outputDir, 'main', `${visualCase.name}.png`); const diffPath = path.join(outputDir, 'diff', `${visualCase.name}.png`); - await downloadObject(r2, baseline.key, mainPath); + await downloadObjectOp(r2, baseline.key, mainPath); + let diffPixels: number; try { - const diffPixels = await writeDiffPng(mainPath, visualCase.path, diffPath); - const mainKey = prImageKey(prNumber, runId, 'main', visualCase.name); - const diffKey = prImageKey(prNumber, runId, 'diff', visualCase.name); - await putFile(r2, mainKey, mainPath); - await putFile(r2, diffKey, diffPath); - - return { - name: visualCase.name, - status: diffPixels > 0 ? 'changed' : 'unchanged', - diffPixels, - baselineSha: baseline.sha, - baselineBehindBy: baseline.behindBy, - mainUrl: publicUrl(r2, mainKey), - prUrl: publicUrl(r2, prKey), - diffUrl: publicUrl(r2, diffKey), - }; + diffPixels = await writeDiffPngOp(mainPath, visualCase.path, diffPath); } catch (error) { return { name: visualCase.name, status: 'failed', - baselineSha: baseline.sha, - baselineBehindBy: baseline.behindBy, - mainUrl: publicUrl(r2, baseline.key), + ...(baseline == null + ? {} + : { + baselineSha: baseline.sha, + baselineBehindBy: baseline.behindBy, + mainUrl: publicUrl(r2, baseline.key), + }), prUrl: publicUrl(r2, prKey), error: error instanceof Error ? error.message : String(error), }; } + + const mainKey = prImageKey(prNumber, runId, 'main', visualCase.name); + const diffKey = prImageKey(prNumber, runId, 'diff', visualCase.name); + await putFileOp(r2, mainKey, mainPath); + await putFileOp(r2, diffKey, diffPath); + + return { + name: visualCase.name, + status: diffPixels > 0 ? 'changed' : 'unchanged', + diffPixels, + baselineSha: baseline.sha, + baselineBehindBy: baseline.behindBy, + mainUrl: publicUrl(r2, mainKey), + prUrl: publicUrl(r2, prKey), + diffUrl: publicUrl(r2, diffKey), + }; } async function writeDiffPng(mainPath: string, prPath: string, diffPath: string): Promise { @@ -425,6 +455,29 @@ function setPixel(png: PNG, x: number, y: number, color: readonly [number, numbe png.data[index + 3] = color[3]; } +function validatePngBuffer(buffer: Buffer, filePath: string): void { + assertPngHeaderSize(buffer, filePath); + const png = PNG.sync.read(buffer); + assertPngSize(png, filePath); +} + +function assertPngHeaderSize(buffer: Buffer, filePath: string): void { + if (buffer.length < 24) { + throw new Error(`Visual case ${filePath} is not a valid PNG file`); + } + const signature = buffer.subarray(0, 8); + const expectedSignature = Buffer.from([137, 80, 78, 71, 13, 10, 26, 10]); + if (!signature.equals(expectedSignature)) { + throw new Error(`Visual case ${filePath} is not a valid PNG file`); + } + const ihdrLength = buffer.readUInt32BE(8); + const chunkType = buffer.toString('ascii', 12, 16); + if (ihdrLength !== 13 || chunkType !== 'IHDR') { + throw new Error(`Visual case ${filePath} is not a valid PNG file`); + } + assertPngPixels(buffer.readUInt32BE(16), buffer.readUInt32BE(20), filePath); +} + function assertPngSize(png: PNG, filePath: string): void { assertPngPixels(png.width, png.height, filePath); } diff --git a/e2e/tests/visual-report.test.ts b/e2e/tests/visual-report.test.ts index 693aae7ee..1ca355397 100644 --- a/e2e/tests/visual-report.test.ts +++ b/e2e/tests/visual-report.test.ts @@ -1,9 +1,14 @@ +import { tmpdir } from 'node:os'; +import path from 'node:path'; +import { mkdtemp, mkdir, readFile, rm, writeFile } from 'node:fs/promises'; + import { describe, expect, test } from 'vitest'; import { PNG } from 'pngjs'; import { assertPngPixels, + compareCase, diffBoxesFromMask, drawBox, mergeDiffBoxes, @@ -19,6 +24,245 @@ describe('visual report PNG sizing', () => { /maximum allowed is 4000000 pixels/, ); }); + + test('malformed screenshots fail one case without preventing later valid cases', async () => { + const workDir = await mkdtemp(path.join(tmpdir(), 'visual-report-')); + try { + const outputDir = path.join(workDir, 'output'); + const goodPath = path.join(workDir, 'visual-good.png'); + const badPath = path.join(workDir, 'visual-bad.png'); + const pngBuffer = PNG.sync.write(createFilledPng(2, 2)); + await writeFile(goodPath, pngBuffer); + await writeFile(badPath, Buffer.from('not-a-png')); + + const r2 = { + bucket: 'visual-bucket', + publicOrigin: 'https://example.invalid', + client: {} as never, + }; + const ops = { + putFile: async () => {}, + findBaseline: async () => ({ sha: 'a'.repeat(40), key: 'baseline/visual-good.png', behindBy: 0 }), + downloadObject: async (_r2: unknown, _key: string, outputPath: string) => { + await mkdir(path.dirname(outputPath), { recursive: true }); + await writeFile(outputPath, pngBuffer); + }, + writeDiffPng: async (_mainPath: string, prPath: string, diffPath: string) => { + await mkdir(path.dirname(diffPath), { recursive: true }); + await writeFile(diffPath, await readFile(prPath)); + return 0; + }, + }; + + const malformed = await compareCase( + { + r2, + prNumber: '12', + runId: '34', + headSha: 'b'.repeat(40), + visualCase: { name: 'visual-bad', path: badPath }, + candidateShas: ['c'.repeat(40)], + outputDir, + }, + ops, + ); + const valid = await compareCase( + { + r2, + prNumber: '12', + runId: '34', + headSha: 'b'.repeat(40), + visualCase: { name: 'visual-good', path: goodPath }, + candidateShas: ['c'.repeat(40)], + outputDir, + }, + ops, + ); + + expect(malformed.status).toBe('failed'); + expect(malformed.name).toBe('visual-bad'); + expect(valid).toMatchObject({ + name: 'visual-good', + status: 'unchanged', + prUrl: 'https://example.invalid/visual-regression/pr-12/34/pr/visual-good.png', + }); + } finally { + await rm(workDir, { recursive: true, force: true }); + } + }); + + test('required R2 failures propagate instead of returning a failed case', async () => { + const workDir = await mkdtemp(path.join(tmpdir(), 'visual-report-')); + try { + const outputDir = path.join(workDir, 'output'); + const goodPath = path.join(workDir, 'visual-good.png'); + const pngBuffer = PNG.sync.write(createFilledPng(2, 2)); + await writeFile(goodPath, pngBuffer); + + const r2 = { + bucket: 'visual-bucket', + publicOrigin: 'https://example.invalid', + client: {} as never, + }; + + await expect(compareCase( + { + r2, + prNumber: '12', + runId: '34', + headSha: 'b'.repeat(40), + visualCase: { name: 'visual-good', path: goodPath }, + candidateShas: ['c'.repeat(40)], + outputDir, + }, + { + putFile: async () => {}, + findBaseline: async () => ({ sha: 'a'.repeat(40), key: 'baseline/visual-good.png', behindBy: 0 }), + downloadObject: async () => { + throw new Error('download failed'); + }, + writeDiffPng: async () => 0, + }, + )).rejects.toThrow('download failed'); + } finally { + await rm(workDir, { recursive: true, force: true }); + } + }); + + test('missing screenshot files propagate instead of returning a failed case', async () => { + const workDir = await mkdtemp(path.join(tmpdir(), 'visual-report-')); + try { + const outputDir = path.join(workDir, 'output'); + const missingPath = path.join(workDir, 'visual-missing.png'); + + const r2 = { + bucket: 'visual-bucket', + publicOrigin: 'https://example.invalid', + client: {} as never, + }; + + await expect(compareCase( + { + r2, + prNumber: '12', + runId: '34', + headSha: 'b'.repeat(40), + visualCase: { name: 'visual-missing', path: missingPath }, + candidateShas: ['c'.repeat(40)], + outputDir, + }, + { + putFile: async () => {}, + findBaseline: async () => null, + downloadObject: async () => { + throw new Error('download should not run'); + }, + writeDiffPng: async () => { + throw new Error('diff should not run'); + }, + }, + )).rejects.toThrow(/ENOENT|no such file/i); + } finally { + await rm(workDir, { recursive: true, force: true }); + } + }); + + test('malformed screenshots without a baseline fail before any PR upload', async () => { + const workDir = await mkdtemp(path.join(tmpdir(), 'visual-report-')); + try { + const outputDir = path.join(workDir, 'output'); + const badPath = path.join(workDir, 'visual-bad.png'); + await writeFile(badPath, Buffer.from('not-a-png')); + + const uploadedKeys: string[] = []; + const r2 = { + bucket: 'visual-bucket', + publicOrigin: 'https://example.invalid', + client: {} as never, + }; + + const result = await compareCase( + { + r2, + prNumber: '12', + runId: '34', + headSha: 'b'.repeat(40), + visualCase: { name: 'visual-bad', path: badPath }, + candidateShas: ['c'.repeat(40)], + outputDir, + }, + { + putFile: async (_r2: unknown, key: string) => { + uploadedKeys.push(key); + }, + findBaseline: async () => null, + downloadObject: async () => { + throw new Error('download should not run'); + }, + writeDiffPng: async () => { + throw new Error('diff should not run'); + }, + }, + ); + + expect(result).toMatchObject({ + name: 'visual-bad', + status: 'failed', + }); + expect(uploadedKeys).toEqual([]); + } finally { + await rm(workDir, { recursive: true, force: true }); + } + }); + + test('huge-dimension PNG headers fail before decode and upload', async () => { + const workDir = await mkdtemp(path.join(tmpdir(), 'visual-report-')); + try { + const outputDir = path.join(workDir, 'output'); + const badPath = path.join(workDir, 'visual-huge.png'); + await writeFile(badPath, createHugeHeaderPng(100_000, 100_000)); + + const uploadedKeys: string[] = []; + const r2 = { + bucket: 'visual-bucket', + publicOrigin: 'https://example.invalid', + client: {} as never, + }; + + const result = await compareCase( + { + r2, + prNumber: '12', + runId: '34', + headSha: 'b'.repeat(40), + visualCase: { name: 'visual-huge', path: badPath }, + candidateShas: ['c'.repeat(40)], + outputDir, + }, + { + putFile: async (_r2: unknown, key: string) => { + uploadedKeys.push(key); + }, + findBaseline: async () => null, + downloadObject: async () => { + throw new Error('download should not run'); + }, + writeDiffPng: async () => { + throw new Error('diff should not run'); + }, + }, + ); + + expect(result).toMatchObject({ + name: 'visual-huge', + status: 'failed', + error: expect.stringMatching(/maximum allowed is 4000000 pixels/), + }); + expect(uploadedKeys).toEqual([]); + } finally { + await rm(workDir, { recursive: true, force: true }); + } + }); }); describe('visual diff box extraction', () => { @@ -102,6 +346,35 @@ function createMask(width: number, height: number, pixels: ReadonlyArray { + const workflow = await readFile(new URL("../.github/workflows/visual-pr-comment.yml", import.meta.url), "utf8"); + const manifestStep = workflow.match(/- name: Read capture manifest[\s\S]*?- name: Validate live PR state for trusted checkout/u)?.[0]; + const trustedPrStep = workflow.match(/- name: Validate live PR state for trusted checkout[\s\S]*?- name: Checkout trusted base/u)?.[0]; + + assert.ok(manifestStep, "Expected Read capture manifest step block in workflow"); + assert.ok(trustedPrStep, "Expected trusted-pr validation step block in workflow"); + assert.match(manifestStep, /encoded_head=.*\$head\|@uri/u); + assert.match(manifestStep, /pulls\?state=open&head=\$encoded_head&per_page=100/); + assert.match(manifestStep, /jq -r --arg repo "\$source_head_repository"/u); + assert.doesNotMatch(manifestStep, /select\(\.head\.sha == \$sha/u); + assert.match(manifestStep, /match_count=.*wc -w/u); + assert.match(manifestStep, /if \[ "\$match_count" -gt 1 \]; then/u); + assert.match(manifestStep, /found \$match_count matches/); + assert.match(manifestStep, /pr_number="\$manifest_pr_number"/u); + assert.match(trustedPrStep, /Skipping stale visual artifact for \$ARTIFACT_HEAD_SHA; current PR head is \$current_head\./u); +}); + test("isPendingApprovalRun matches approval-gated fork PR runs from GitHub's captured payload shape", () => { const pull = { number: 2683, @@ -107,6 +127,23 @@ test("isPendingApprovalRun rejects runs outside the allowlist or without action_ false, ); + assert.equal( + isPendingApprovalRun( + { + id: 26273463771, + name: "Visual PR Capture", + event: "pull_request", + status: "completed", + conclusion: "action_required", + head_sha: "734076155c44e569304856590019cea54506fdab", + path: ".github/workflows/visual-pr-capture.yml@main", + pull_requests: [], + }, + pull, + ), + false, + ); + assert.equal( isPendingApprovalRun( { @@ -125,6 +162,56 @@ test("isPendingApprovalRun rejects runs outside the allowlist or without action_ ); }); +test("isPendingApprovalRun approves visual capture only for strict web source changes", () => { + const pull = { + number: 2683, + state: "open", + changed_files: 1, + head: { + ref: "fix/button-copy", + sha: "734076155c44e569304856590019cea54506fdab", + repo: { full_name: "someone/open-design" }, + }, + base: { + ref: "main", + sha: "4cd93a5c7a7b0db1961c854e55f8e0e6b1b45542", + repo: { full_name: "nexu-io/open-design" }, + }, + }; + const run = { + id: 26273463771, + name: "Visual PR Capture", + event: "pull_request", + status: "completed", + conclusion: "action_required", + head_sha: "734076155c44e569304856590019cea54506fdab", + path: ".github/workflows/visual-pr-capture.yml@main", + pull_requests: [], + }; + + assert.equal( + isPendingApprovalRun(run, pull, [ + { filename: "apps/web/src/components/Button.tsx", status: "modified" }, + { filename: "apps/web/src/styles/button.css", status: "modified" }, + ]), + true, + ); + assert.equal( + isPendingApprovalRun(run, pull, [{ filename: "apps/web/public/logo.png", status: "modified" }]), + false, + ); + assert.equal( + isPendingApprovalRun(run, pull, [ + { + filename: "apps/web/src/components/Button.tsx", + previous_filename: "scripts/build.ts", + status: "renamed", + }, + ]), + false, + ); +}); + test("runTargetsPullRequest accepts empty run.pull_requests only when the head SHA maps to this one open PR", () => { const pull = { number: 2683, @@ -517,6 +604,68 @@ test("listPendingApprovalRuns paginates all pull_request runs for the head SHA a ); }); +test("listPendingApprovalRuns applies strict changed-path filtering only to visual capture", async () => { + const pull = { + number: 2683, + state: "open", + changed_files: 1, + head: { + ref: "fix/readme-copy", + sha: "734076155c44e569304856590019cea54506fdab", + repo: { full_name: "someone/open-design" }, + }, + base: { + ref: "main", + sha: "4cd93a5c7a7b0db1961c854e55f8e0e6b1b45542", + repo: { full_name: "nexu-io/open-design" }, + }, + }; + const workflowRuns = [ + { + id: 26273463769, + name: "CI", + event: "pull_request", + head_branch: pull.head.ref, + head_repository: pull.head.repo, + status: "completed", + conclusion: "action_required", + head_sha: pull.head.sha, + path: ".github/workflows/ci.yml@main", + pull_requests: [], + }, + { + id: 26273463770, + name: "Visual PR Capture", + event: "pull_request", + head_branch: pull.head.ref, + head_repository: pull.head.repo, + status: "completed", + conclusion: "action_required", + head_sha: pull.head.sha, + path: ".github/workflows/visual-pr-capture.yml@main", + pull_requests: [], + }, + ]; + const deps = { + loadWorkflowRunsResponsePage: async () => ({ workflow_runs: workflowRuns }), + loadPullRequestsForHeadSha: async () => [pull], + }; + + assert.deepEqual( + (await listPendingApprovalRuns("nexu-io/open-design", pull, [{ filename: "README.md", status: "modified" }], deps)).map((run) => run.id), + [26273463769], + ); + assert.deepEqual( + (await listPendingApprovalRuns( + "nexu-io/open-design", + pull, + [{ filename: "apps/web/src/components/Button.tsx", status: "modified" }], + deps, + )).map((run) => run.id), + [26273463769, 26273463770], + ); +}); + test("hasPullApprovalStateDrift ignores base tip churn but still rejects base retargeting and head drift", () => { const pull = { number: 2683, @@ -582,6 +731,17 @@ test("isAllowedChangedPath allows ordinary app and package source/test paths whi assert.equal(isAllowedChangedPath("apps/packaged/vitest.config.ts"), false); }); +test("isAllowedVisualCaptureChangedPath is limited to web ts tsx and css source", () => { + assert.equal(isAllowedVisualCaptureChangedPath("apps/web/src/app/page.tsx"), true); + assert.equal(isAllowedVisualCaptureChangedPath("apps/web/src/lib/theme.ts"), true); + assert.equal(isAllowedVisualCaptureChangedPath("apps/web/src/components/Button.css"), true); + assert.equal(isAllowedVisualCaptureChangedPath("apps/web/src/assets/icon.svg"), false); + assert.equal(isAllowedVisualCaptureChangedPath("apps/web/public/logo.png"), false); + assert.equal(isAllowedVisualCaptureChangedPath("apps/web/package.json"), false); + assert.equal(isAllowedVisualCaptureChangedPath("apps/web/tests/Button.test.tsx"), false); + assert.equal(isAllowedVisualCaptureChangedPath("packages/contracts/src/api.ts"), false); +}); + test("waitForPendingApprovalRuns retries until action_required runs appear and keeps polling through the retry window", async () => { const run = { id: 26273463769, diff --git a/scripts/approve-fork-pr-workflows.ts b/scripts/approve-fork-pr-workflows.ts index 1342b40fb..76de7e875 100644 --- a/scripts/approve-fork-pr-workflows.ts +++ b/scripts/approve-fork-pr-workflows.ts @@ -78,6 +78,8 @@ const allowedWorkflowPaths = new Set([ ".github/workflows/visual-pr-verify.yml", ]); +const visualPrCaptureWorkflowPath = ".github/workflows/visual-pr-capture.yml"; + export function normalizeWorkflowPath(path: string): string { const suffixIndex = path.indexOf("@"); return suffixIndex >= 0 ? path.slice(0, suffixIndex) : path; @@ -114,6 +116,10 @@ export function isAllowedChangedPath(path: string): boolean { ); } +export function isAllowedVisualCaptureChangedPath(path: string): boolean { + return /^apps\/web\/src\/.+\.(?:css|ts|tsx)$/.test(path); +} + export function isDeniedChangedPath(path: string): boolean { return ( path.startsWith(".github/") || @@ -151,12 +157,23 @@ function changedPathSet(file: PullRequestFile): string[] { return [file.filename, file.previous_filename].filter((path): path is string => Boolean(path)); } -export function isPendingApprovalRun(run: WorkflowRun, pull: PullRequest): boolean { +function allChangedPathsMatch(files: PullRequestFile[], predicate: (path: string) => boolean): boolean { + return files.every((file) => changedPathSet(file).every(predicate)); +} + +function workflowAllowsChangedFiles(workflowPath: string, files: PullRequestFile[] | undefined): boolean { + if (workflowPath !== visualPrCaptureWorkflowPath) return true; + return files != null && files.length > 0 && allChangedPathsMatch(files, isAllowedVisualCaptureChangedPath); +} + +export function isPendingApprovalRun(run: WorkflowRun, pull: PullRequest, files?: PullRequestFile[]): boolean { + const workflowPath = normalizeWorkflowPath(run.path); return ( run.head_sha === pull.head.sha && run.event === "pull_request" && (run.status === "action_required" || run.conclusion === "action_required") && - allowedWorkflowPaths.has(normalizeWorkflowPath(run.path)) + allowedWorkflowPaths.has(workflowPath) && + workflowAllowsChangedFiles(workflowPath, files) ); } @@ -330,8 +347,11 @@ async function listWorkflowRunsForHeadSha( export async function listPendingApprovalRuns( repo: string, pull: PullRequest, - deps: ListPendingApprovalRunsDeps = {}, + filesOrDeps: PullRequestFile[] | ListPendingApprovalRunsDeps = [], + maybeDeps: ListPendingApprovalRunsDeps = {}, ): Promise { + const files = Array.isArray(filesOrDeps) ? filesOrDeps : undefined; + const deps = Array.isArray(filesOrDeps) ? maybeDeps : filesOrDeps; const loadWorkflowRunsResponsePage = deps.loadWorkflowRunsResponsePage ?? ((path: string) => github(path)); const loadPullRequestsForHeadSha = deps.loadPullRequestsForHeadSha ?? ((currentRepo: string, headSha: string) => listPullRequestsForHeadSha(currentRepo, headSha)); @@ -360,7 +380,7 @@ export async function listPendingApprovalRuns( return workflowRuns.filter( (run) => - isPendingApprovalRun(run, pull) && + isPendingApprovalRun(run, pull, files) && runTargetsPullRequest(run, pull, associatedPullsForHeadSha, associatedPullsForHeadRef), ); } @@ -413,7 +433,7 @@ async function main(): Promise { return; } - const pendingRuns = await waitForPendingApprovalRuns(() => listPendingApprovalRuns(repo, pull)); + const pendingRuns = await waitForPendingApprovalRuns(() => listPendingApprovalRuns(repo, pull, files)); if (pendingRuns.length === 0) { console.log(`No action_required pull_request workflow runs found for PR #${prNumber} at ${pull.head.sha}.`);