From 6689a1b14ef29c405be106d542d0397d6967f313 Mon Sep 17 00:00:00 2001 From: Christian Byrne Date: Wed, 4 Mar 2026 22:09:31 -0800 Subject: [PATCH] fix: split perf report workflow for fork PR support (#9382) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit ## Summary Perf report workflow fails on fork PRs because `GITHUB_TOKEN` is read-only for forks, causing "Resource not accessible by integration" on the PR comment step. ## Changes - **What**: Split `ci-perf-report.yaml` into a data-collection workflow + a `workflow_run`-triggered reporter (`pr-perf-report.yaml`), matching the existing `ci-size-data`/`pr-size-report` pattern. Added fork PR permissions guidance to `.github/AGENTS.md`. - **ci-perf-report.yaml**: Removed the `report` job and `pull-requests: write` permission. Added PR metadata (number + base branch) artifact upload. - **pr-perf-report.yaml** (new): Triggered by `workflow_run` on the perf workflow. Downloads metrics + metadata artifacts, generates report, posts PR comment with write permissions from the default-branch context. ## Review Focus - The two-workflow split follows the same pattern as `ci-size-data.yaml` → `pr-size-report.yaml`, which already works for fork PRs. - The `workflow_run` trigger runs in the base repo context per [GitHub Security Lab guidance](https://securitylab.github.com/resources/github-actions-preventing-pwn-requests/), so it safely has write permissions even for fork PRs. - AGENTS.md guidance documents this pattern to prevent recurrence. Fixes the failure seen in https://github.com/Comfy-Org/ComfyUI_frontend/actions/runs/22684230751/job/65763595989?pr=9380 ┆Issue is synchronized with this [Notion page](https://www.notion.so/PR-9382-fix-split-perf-report-workflow-for-fork-PR-support-3196d73d365081b29b35ed354e7789e2) by [Unito](https://www.unito.io) --- .github/AGENTS.md | 8 ++ .../post-pr-report-comment/action.yaml | 35 ++++++ .github/workflows/ci-perf-report.yaml | 86 ++------------- .github/workflows/pr-perf-report.yaml | 102 ++++++++++++++++++ .github/workflows/pr-size-report.yaml | 100 +++++++++++------ 5 files changed, 225 insertions(+), 106 deletions(-) create mode 100644 .github/actions/post-pr-report-comment/action.yaml create mode 100644 .github/workflows/pr-perf-report.yaml diff --git a/.github/AGENTS.md b/.github/AGENTS.md index 36ee825b4d..9da6f75335 100644 --- a/.github/AGENTS.md +++ b/.github/AGENTS.md @@ -13,3 +13,11 @@ This automated review performs comprehensive analysis: - Integration concerns For implementation details, see `.claude/commands/comprehensive-pr-review.md`. + +## GitHub Actions: Fork PR Permissions + +Fork PRs get a **read-only `GITHUB_TOKEN`** — no PR comments, no secret access, no pushing. + +Any workflow that needs write access must use the **two-workflow split**: a `pull_request`-triggered `ci-*.yaml` uploads artifacts (including PR metadata), then a `workflow_run`-triggered `pr-*.yaml` downloads them and posts comments with write permissions. See `ci-size-data` → `pr-size-report` or `ci-perf-report` → `pr-perf-report`. Use `.github/actions/post-pr-report-comment` for the comment step. + +Never write PR comments directly from `pull_request` workflows or use `pull_request_target` to run untrusted code. diff --git a/.github/actions/post-pr-report-comment/action.yaml b/.github/actions/post-pr-report-comment/action.yaml new file mode 100644 index 0000000000..3adaae7f83 --- /dev/null +++ b/.github/actions/post-pr-report-comment/action.yaml @@ -0,0 +1,35 @@ +name: Post PR Report Comment +description: Reads a markdown report file and posts/updates a single idempotent comment on a PR. + +inputs: + pr-number: + description: PR number to comment on + required: true + report-file: + description: Path to the markdown report file + required: true + comment-marker: + description: HTML comment marker for idempotent matching + required: true + token: + description: GitHub token with pull-requests write permission + required: true + +runs: + using: composite + steps: + - name: Read report + id: report + uses: juliangruber/read-file-action@b549046febe0fe86f8cb4f93c24e284433f9ab58 # v1.1.7 + with: + path: ${{ inputs.report-file }} + + - name: Create or update PR comment + uses: actions-cool/maintain-one-comment@4b2dbf086015f892dcb5e8c1106f5fccd6c1476b # v3.2.0 + with: + token: ${{ inputs.token }} + number: ${{ inputs.pr-number }} + body: | + ${{ steps.report.outputs.content }} + ${{ inputs.comment-marker }} + body-include: ${{ inputs.comment-marker }} diff --git a/.github/workflows/ci-perf-report.yaml b/.github/workflows/ci-perf-report.yaml index c0de7b8fff..724034a8a4 100644 --- a/.github/workflows/ci-perf-report.yaml +++ b/.github/workflows/ci-perf-report.yaml @@ -14,7 +14,6 @@ concurrency: permissions: contents: read - pull-requests: write jobs: perf-tests: @@ -56,81 +55,16 @@ jobs: retention-days: 30 if-no-files-found: warn - report: - needs: perf-tests - if: github.event_name == 'pull_request' - runs-on: ubuntu-latest - permissions: - actions: read - contents: read - pull-requests: write - - steps: - - name: Checkout repository - uses: actions/checkout@v6 - - - name: Setup Node - uses: actions/setup-node@v6 - with: - node-version: 22 - - - name: Download PR perf metrics - continue-on-error: true - uses: actions/download-artifact@v7 - with: - name: perf-metrics - path: test-results/ - - - name: Download baseline perf metrics - uses: dawidd6/action-download-artifact@0bd50d53a6d7fb5cb921e607957e9cc12b4ce392 # v12 - with: - branch: ${{ github.event.pull_request.base.ref }} - workflow: ci-perf-report.yaml - event: push - name: perf-metrics - path: temp/perf-baseline/ - if_no_artifact_found: warn - - - name: Download historical perf baselines - continue-on-error: true + - name: Save PR metadata + if: github.event_name == 'pull_request' run: | - RUNS=$(gh api \ - "/repos/${{ github.repository }}/actions/workflows/ci-perf-report.yaml/runs?branch=${{ github.event.pull_request.base.ref }}&event=push&status=success&per_page=5" \ - --jq '.workflow_runs[].id' || true) + mkdir -p temp/perf-meta + echo "${{ github.event.number }}" > temp/perf-meta/number.txt + echo "${{ github.event.pull_request.base.ref }}" > temp/perf-meta/base.txt - if [ -z "$RUNS" ]; then - echo "No historical runs available" - exit 0 - fi - - mkdir -p temp/perf-history - INDEX=0 - for RUN_ID in $RUNS; do - DIR="temp/perf-history/$INDEX" - mkdir -p "$DIR" - gh run download "$RUN_ID" -n perf-metrics -D "$DIR/" 2>/dev/null || true - INDEX=$((INDEX + 1)) - done - - echo "Downloaded $(ls temp/perf-history/*/perf-metrics.json 2>/dev/null | wc -l) historical baselines" - env: - GH_TOKEN: ${{ secrets.GITHUB_TOKEN }} - - - name: Generate perf report - run: npx --yes tsx scripts/perf-report.ts > perf-report.md - - - name: Read perf report - id: perf-report - uses: juliangruber/read-file-action@b549046febe0fe86f8cb4f93c24e284433f9ab58 # v1.1.7 + - name: Upload PR metadata + if: github.event_name == 'pull_request' + uses: actions/upload-artifact@v6 with: - path: ./perf-report.md - - - name: Create or update PR comment - uses: actions-cool/maintain-one-comment@4b2dbf086015f892dcb5e8c1106f5fccd6c1476b # v3.2.0 - with: - token: ${{ secrets.GITHUB_TOKEN }} - number: ${{ github.event.pull_request.number }} - body: | - ${{ steps.perf-report.outputs.content }} - - body-include: '' + name: perf-meta + path: temp/perf-meta/ diff --git a/.github/workflows/pr-perf-report.yaml b/.github/workflows/pr-perf-report.yaml new file mode 100644 index 0000000000..3d13867354 --- /dev/null +++ b/.github/workflows/pr-perf-report.yaml @@ -0,0 +1,102 @@ +name: 'PR: Performance Report' + +on: + workflow_run: + workflows: ['CI: Performance Report'] + types: + - completed + +permissions: + contents: read + pull-requests: write + issues: write + +jobs: + comment: + runs-on: ubuntu-latest + if: > + github.repository == 'Comfy-Org/ComfyUI_frontend' && + github.event.workflow_run.event == 'pull_request' && + github.event.workflow_run.conclusion == 'success' + steps: + - name: Checkout repository + uses: actions/checkout@v6 + + - name: Setup Node + uses: actions/setup-node@v6 + with: + node-version: 22 + + - name: Download PR metadata + uses: dawidd6/action-download-artifact@0bd50d53a6d7fb5cb921e607957e9cc12b4ce392 # v12 + with: + name: perf-meta + run_id: ${{ github.event.workflow_run.id }} + path: temp/perf-meta/ + + - name: Resolve and validate PR metadata + id: pr-meta + uses: actions/github-script@v8 + with: + script: | + const fs = require('fs'); + const artifactPr = Number(fs.readFileSync('temp/perf-meta/number.txt', 'utf8').trim()); + const artifactBase = fs.readFileSync('temp/perf-meta/base.txt', 'utf8').trim(); + + // Resolve PR from trusted workflow context + let pr = context.payload.workflow_run.pull_requests?.[0]; + if (!pr) { + const { data: prs } = await github.rest.repos.listPullRequestsAssociatedWithCommit({ + owner: context.repo.owner, + repo: context.repo.repo, + commit_sha: context.payload.workflow_run.head_sha, + }); + pr = prs.find(p => p.state === 'open'); + } + + if (!pr) { + core.setFailed('Unable to resolve PR from workflow_run context.'); + return; + } + + if (Number(pr.number) !== artifactPr) { + core.setFailed(`Artifact PR number (${artifactPr}) does not match trusted context (${pr.number}).`); + return; + } + + const trustedBase = pr.base?.ref; + if (!trustedBase || artifactBase !== trustedBase) { + core.setFailed(`Artifact base (${artifactBase}) does not match trusted context (${trustedBase}).`); + return; + } + + core.setOutput('number', String(pr.number)); + core.setOutput('base', trustedBase); + + - name: Download PR perf metrics + uses: dawidd6/action-download-artifact@0bd50d53a6d7fb5cb921e607957e9cc12b4ce392 # v12 + with: + name: perf-metrics + run_id: ${{ github.event.workflow_run.id }} + path: test-results/ + + - name: Download baseline perf metrics + uses: dawidd6/action-download-artifact@0bd50d53a6d7fb5cb921e607957e9cc12b4ce392 # v12 + with: + branch: ${{ steps.pr-meta.outputs.base }} + workflow: ci-perf-report.yaml + event: push + name: perf-metrics + path: temp/perf-baseline/ + if_no_artifact_found: warn + + - name: Generate perf report + run: npx --yes tsx scripts/perf-report.ts > perf-report.md + + - name: Post PR comment + uses: ./.github/actions/post-pr-report-comment + with: + pr-number: ${{ steps.pr-meta.outputs.number }} + report-file: ./perf-report.md + comment-marker: '' + token: ${{ secrets.GITHUB_TOKEN }} diff --git a/.github/workflows/pr-size-report.yaml b/.github/workflows/pr-size-report.yaml index 769ce0e1a7..2b999298b8 100644 --- a/.github/workflows/pr-size-report.yaml +++ b/.github/workflows/pr-size-report.yaml @@ -45,28 +45,76 @@ jobs: run_id: ${{ github.event_name == 'workflow_dispatch' && inputs.run_id || github.event.workflow_run.id }} path: temp/size - - name: Set PR number - id: pr-number - run: | - if [ "${{ github.event_name }}" == "workflow_dispatch" ]; then - echo "content=${{ inputs.pr_number }}" >> $GITHUB_OUTPUT - else - echo "content=$(cat temp/size/number.txt)" >> $GITHUB_OUTPUT - fi + - name: Resolve and validate PR metadata + id: pr-meta + uses: actions/github-script@v8 + with: + script: | + const fs = require('fs'); - - name: Set base branch - id: pr-base - run: | - if [ "${{ github.event_name }}" == "workflow_dispatch" ]; then - echo "content=main" >> $GITHUB_OUTPUT - else - echo "content=$(cat temp/size/base.txt)" >> $GITHUB_OUTPUT - fi + // workflow_dispatch: validate artifact metadata against API-resolved PR + if (context.eventName === 'workflow_dispatch') { + const pullNumber = Number('${{ inputs.pr_number }}'); + const { data: dispatchPr } = await github.rest.pulls.get({ + owner: context.repo.owner, + repo: context.repo.repo, + pull_number: pullNumber, + }); + + const artifactPr = Number(fs.readFileSync('temp/size/number.txt', 'utf8').trim()); + const artifactBase = fs.readFileSync('temp/size/base.txt', 'utf8').trim(); + + if (artifactPr !== dispatchPr.number) { + core.setFailed(`Artifact PR number (${artifactPr}) does not match dispatch PR (${dispatchPr.number}).`); + return; + } + if (artifactBase !== dispatchPr.base.ref) { + core.setFailed(`Artifact base (${artifactBase}) does not match dispatch PR base (${dispatchPr.base.ref}).`); + return; + } + + core.setOutput('number', String(dispatchPr.number)); + core.setOutput('base', dispatchPr.base.ref); + return; + } + + // workflow_run: validate artifact metadata against trusted context + const artifactPr = Number(fs.readFileSync('temp/size/number.txt', 'utf8').trim()); + const artifactBase = fs.readFileSync('temp/size/base.txt', 'utf8').trim(); + + let pr = context.payload.workflow_run.pull_requests?.[0]; + if (!pr) { + const { data: prs } = await github.rest.repos.listPullRequestsAssociatedWithCommit({ + owner: context.repo.owner, + repo: context.repo.repo, + commit_sha: context.payload.workflow_run.head_sha, + }); + pr = prs.find(p => p.state === 'open'); + } + + if (!pr) { + core.setFailed('Unable to resolve PR from workflow_run context.'); + return; + } + + if (Number(pr.number) !== artifactPr) { + core.setFailed(`Artifact PR number (${artifactPr}) does not match trusted context (${pr.number}).`); + return; + } + + const trustedBase = pr.base?.ref; + if (!trustedBase || artifactBase !== trustedBase) { + core.setFailed(`Artifact base (${artifactBase}) does not match trusted context (${trustedBase}).`); + return; + } + + core.setOutput('number', String(pr.number)); + core.setOutput('base', trustedBase); - name: Download previous size data uses: dawidd6/action-download-artifact@0bd50d53a6d7fb5cb921e607957e9cc12b4ce392 # v12 with: - branch: ${{ steps.pr-base.outputs.content }} + branch: ${{ steps.pr-meta.outputs.base }} workflow: ci-size-data.yaml event: push name: size-data @@ -76,18 +124,10 @@ jobs: - name: Generate size report run: node scripts/size-report.js > size-report.md - - name: Read size report - id: size-report - uses: juliangruber/read-file-action@b549046febe0fe86f8cb4f93c24e284433f9ab58 # v1.1.7 - with: - path: ./size-report.md - - - name: Create or update PR comment - uses: actions-cool/maintain-one-comment@4b2dbf086015f892dcb5e8c1106f5fccd6c1476b # v3.2.0 + - name: Post PR comment + uses: ./.github/actions/post-pr-report-comment with: + pr-number: ${{ steps.pr-meta.outputs.number }} + report-file: ./size-report.md + comment-marker: '' token: ${{ secrets.GITHUB_TOKEN }} - number: ${{ steps.pr-number.outputs.content }} - body: | - ${{ steps.size-report.outputs.content }} - - body-include: ''