From 4d60c1fc017fbdf86a7fdafae4b178d6dd7e8562 Mon Sep 17 00:00:00 2001 From: Jackson Weber Date: Wed, 3 Jun 2026 11:02:55 -0700 Subject: [PATCH 1/2] ci(perf): post sticky comment for fork PRs via workflow_run The Performance workflow runs in the PR context, which only grants a read-only GITHUB_TOKEN for pull requests from forks. As a result the "Post sticky PR comment" step was gated off for cross-repo PRs and the perf comparison never appeared on those PRs (e.g. microsoft/opentelemetry-distro-python#182). Move the comment posting into a new "Performance Comment" workflow triggered by workflow_run, which executes in the base repository context with pull-requests: write regardless of the PR's origin. The benchmarking workflow now records the PR number in pr-number.txt and uploads it alongside report.md so the follow-up workflow can target the correct PR. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- .github/workflows/performance-comment.yml | 72 +++++++++++++++++++++++ .github/workflows/performance.yml | 17 ++---- 2 files changed, 78 insertions(+), 11 deletions(-) create mode 100644 .github/workflows/performance-comment.yml diff --git a/.github/workflows/performance-comment.yml b/.github/workflows/performance-comment.yml new file mode 100644 index 00000000..3e886c44 --- /dev/null +++ b/.github/workflows/performance-comment.yml @@ -0,0 +1,72 @@ +name: Performance Comment + +# Posts the perf comparison as a sticky PR comment. +# +# This runs in the *base* repository's context via `workflow_run`, which +# means it has a writable `GITHUB_TOKEN` even for pull requests opened from +# forks. The Performance workflow itself runs in the (potentially untrusted) +# PR context and only produces an artifact — no PR write permissions there. + +on: + workflow_run: + workflows: ["Performance"] + types: + - completed + +permissions: + contents: read + pull-requests: write + actions: read + +jobs: + comment: + name: Post sticky PR comment + runs-on: ubuntu-latest + if: github.event.workflow_run.event == 'pull_request' + + steps: + - name: Download perf-results artifact + id: download + uses: actions/download-artifact@v4 + with: + name: perf-results + path: perf-results + run-id: ${{ github.event.workflow_run.id }} + github-token: ${{ secrets.GITHUB_TOKEN }} + + - name: Resolve PR number + id: pr + run: | + set -euo pipefail + if [ -f perf-results/pr-number.txt ]; then + pr="$(tr -d '[:space:]' < perf-results/pr-number.txt)" + else + pr="" + fi + + if [ -z "$pr" ]; then + echo "Could not determine PR number; skipping comment." >&2 + echo "number=" >> "$GITHUB_OUTPUT" + exit 0 + fi + + echo "number=$pr" >> "$GITHUB_OUTPUT" + + - name: Verify report exists + id: report + if: steps.pr.outputs.number != '' + run: | + if [ -s perf-results/report.md ]; then + echo "found=true" >> "$GITHUB_OUTPUT" + else + echo "found=false" >> "$GITHUB_OUTPUT" + echo "report.md missing or empty; skipping comment." >&2 + fi + + - name: Post sticky PR comment + if: steps.pr.outputs.number != '' && steps.report.outputs.found == 'true' + uses: marocchino/sticky-pull-request-comment@v2 + with: + header: perf-comparison + number: ${{ steps.pr.outputs.number }} + path: perf-results/report.md diff --git a/.github/workflows/performance.yml b/.github/workflows/performance.yml index 6919129d..e956ec62 100644 --- a/.github/workflows/performance.yml +++ b/.github/workflows/performance.yml @@ -94,6 +94,10 @@ jobs: --output "$GITHUB_WORKSPACE/report.md" echo "exit_code=$?" >> "$GITHUB_OUTPUT" + - name: Record PR number + if: always() && github.event_name == 'pull_request' + run: echo "${{ github.event.pull_request.number }}" > "$GITHUB_WORKSPACE/pr-number.txt" + - name: Upload artifacts if: always() uses: actions/upload-artifact@v4 @@ -103,17 +107,8 @@ jobs: pr.json base.json report.md - - - name: Post sticky PR comment - # Skip for PRs from forks — the default GITHUB_TOKEN has read-only - # access in that case and the comment API will reject the call. The - # JSON artifacts and the workflow log still contain the comparison. - if: always() && github.event_name == 'pull_request' && github.event.pull_request.head.repo.full_name == github.repository - continue-on-error: true - uses: marocchino/sticky-pull-request-comment@v2 - with: - header: perf-comparison - path: report.md + pr-number.txt + if-no-files-found: ignore - name: Fail on regression if: steps.compare.outputs.exit_code != '0' From 0b04473eb231a3c6fa8330a0c6e3f6a3e3c6014d Mon Sep 17 00:00:00 2001 From: Jackson Weber Date: Wed, 3 Jun 2026 13:33:03 -0700 Subject: [PATCH 2/2] ci(perf): harden comment workflow per review Address Copilot review feedback on #186: * Treat the perf-results artifact as untrusted. Check out the base repo and regenerate report.md here from base.json/pr.json using the base-branch copy of perf.compare so the markdown posted under the writable GITHUB_TOKEN is never attacker-supplied by a fork PR. * Make actions/download-artifact non-fatal (continue-on-error) and gate every subsequent step on the download outcome, so a cancelled or failed upstream Performance run skips cleanly instead of marking the comment job as failed. * Validate that pr-number.txt contains a positive integer via regex before passing it to the sticky-comment action; skip cleanly if not. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- .github/workflows/performance-comment.yml | 53 +++++++++++++++++++---- 1 file changed, 45 insertions(+), 8 deletions(-) diff --git a/.github/workflows/performance-comment.yml b/.github/workflows/performance-comment.yml index 3e886c44..54d1be53 100644 --- a/.github/workflows/performance-comment.yml +++ b/.github/workflows/performance-comment.yml @@ -6,6 +6,14 @@ name: Performance Comment # means it has a writable `GITHUB_TOKEN` even for pull requests opened from # forks. The Performance workflow itself runs in the (potentially untrusted) # PR context and only produces an artifact — no PR write permissions there. +# +# Security model: +# * The artifact from the PR-context workflow is treated as untrusted +# input. We only consume the JSON benchmark outputs (`base.json` / +# `pr.json`) and the PR number, and we regenerate `report.md` here +# using the base repository's checked-out `perf.compare` so the +# markdown posted under the writable token is never attacker-supplied. +# * The PR number is validated as a positive integer before being used. on: workflow_run: @@ -25,8 +33,14 @@ jobs: if: github.event.workflow_run.event == 'pull_request' steps: + - name: Check out base repository (trusted) + uses: actions/checkout@v4 + - name: Download perf-results artifact id: download + # Don't fail this job if the upstream Performance run was cancelled + # or failed before producing the artifact — we just skip posting. + continue-on-error: true uses: actions/download-artifact@v4 with: name: perf-results @@ -36,31 +50,54 @@ jobs: - name: Resolve PR number id: pr + if: steps.download.outcome == 'success' run: | set -euo pipefail + pr="" if [ -f perf-results/pr-number.txt ]; then pr="$(tr -d '[:space:]' < perf-results/pr-number.txt)" - else - pr="" fi - if [ -z "$pr" ]; then - echo "Could not determine PR number; skipping comment." >&2 + if ! [[ "$pr" =~ ^[1-9][0-9]*$ ]]; then + echo "pr-number.txt missing or not a positive integer (got: '${pr}'); skipping comment." >&2 echo "number=" >> "$GITHUB_OUTPUT" exit 0 fi echo "number=$pr" >> "$GITHUB_OUTPUT" - - name: Verify report exists + - name: Set up Python + if: steps.pr.outputs.number != '' + uses: actions/setup-python@v5 + with: + python-version: "3.12" + + - name: Regenerate report from JSON (trusted code) id: report if: steps.pr.outputs.number != '' run: | - if [ -s perf-results/report.md ]; then + set -euo pipefail + if [ ! -s perf-results/base.json ] || [ ! -s perf-results/pr.json ]; then + echo "base.json or pr.json missing from artifact; skipping comment." >&2 + echo "found=false" >> "$GITHUB_OUTPUT" + exit 0 + fi + + # Use the base repo's perf.compare (trusted) to render the markdown + # from the PR-supplied JSON data, so we never post attacker-supplied + # markdown under the base repo's writable token. + python -m perf.compare \ + --baseline perf-results/base.json \ + --candidate perf-results/pr.json \ + --threshold "${PERF_REGRESSION_THRESHOLD:-15}" \ + --output report.md \ + || true + + if [ -s report.md ]; then echo "found=true" >> "$GITHUB_OUTPUT" else echo "found=false" >> "$GITHUB_OUTPUT" - echo "report.md missing or empty; skipping comment." >&2 + echo "report.md was not produced; skipping comment." >&2 fi - name: Post sticky PR comment @@ -69,4 +106,4 @@ jobs: with: header: perf-comparison number: ${{ steps.pr.outputs.number }} - path: perf-results/report.md + path: report.md