Skip to content

[ci] Skip benchmark PR comments on fork PRs and show results in the job summary - #1943

Open
henryqdineen wants to merge 3 commits into
meta:mainfrom
henryqdineen:hqd-benchmarks-fork-prs
Open

henryqdineen wants to merge 3 commits into
meta:mainfrom
henryqdineen:hqd-benchmarks-fork-prs

Conversation

@henryqdineen

@henryqdineen henryqdineen commented Oct 10, 2026 •

Copy link
Copy Markdown
Collaborator

What changed / motivation ?

The perf and size jobs in benchmarks.yml fail on every PR opened from a fork, so each push to a fork PR sends its author a "[facebook/stylex] PR run failed: benchmarks" email. The benchmarks run fine. The failure is the final Post comment step, which gets 403 Resource not accessible by integration. For PRs from forks, GitHub gives pull_request workflows a read-only GITHUB_TOKEN, so the step can't comment. A PR from a branch in this repo, like #1882, passes both jobs, while the PRs from my fork (#1911, #1914, #1920–#1923, #1928) all fail them.

This change:

  • Skips the Post comment step when the PR comes from a fork (github.event.pull_request.head.repo.full_name == github.repository).
  • Writes the comparison table to $GITHUB_STEP_SUMMARY in both jobs, so fork PRs still show results on the run's summary page.
  • Runs the compare step with yarn -s … run -s so the table no longer includes yarn's banner, $ node ./compare.js …, and Done in … lines, and drops the unneeded -- from the yarn calls so the steps stop logging yarn's deprecation warning.

PRs from branches in this repo work as before. The jobs never failed on regressions, since compare.js only flags them with **!!**, so this removes no gate. A crash in the benchmarks on the PR's code still fails the job.

Linked PR/Issues

Follow-up to #1400. It added permissions: pull-requests: write, which can't fix fork PRs: GitHub doesn't grant write access to fork PRs through the permissions key.

Additional Context

Other repos use the same guard together with a step summary:

If we want the comment on fork PRs too, the safe next step is the two-workflow split that facebook/react's sizebot uses. The pull_request workflow uploads the table as an artifact, and a workflow_run workflow, which gets a write token, posts it. The PR number has to come from the workflow_run event, not the artifact. I kept this PR small, but I'm happy to do that as a follow-up. Switching to pull_request_target isn't safe here, because these jobs run yarn install and scripts from the PR's code (GitHub Security Lab, and the same note in facebook/docusaurus build-perf.yml).

This PR comes from a fork, so its own perf and size checks will show whether the change works: they should pass, and the tables should appear on the run summary.

Pre-flight checklist

🤖 Generated with Claude Code

…ob summary

Fork PRs get a read-only GITHUB_TOKEN under `pull_request`, so the
`Post comment` step 403s and fails the perf and size jobs on every
external PR even though the benchmarks themselves succeed. Guard the
comment step to same-repo PRs and write the comparison table to the
job summary so fork PRs still surface results.

Co-Authored-By: Claude Opus 5.5 <[email protected]>
@vercel

vercel Bot commented Oct 10, 2026

Copy link
Copy Markdown

@henryqdineen is attempting to deploy a commit to the Meta Open Source Team on Vercel.

A member of the Team first needs to authorize it.

@meta-cla meta-cla Bot added the CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. label Oct 10, 2026
henryqdineen and others added 2 commits October 10, 2026 13:32
yarn's own banner, "$ node ./compare.js …", and "Done in …" lines were
captured along with the table into the PR comment and now the job
summary. Dropping the `--` also removes yarn's deprecation warning
from the step log.

Co-Authored-By: Claude Opus 5.5 <[email protected]>
yarn 1 forwards script args without `--` and warns when it's present,
so every benchmark step logged a deprecation warning.

Co-Authored-By: Claude Opus 5.5 <[email protected]>

This branch has not been deployed

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant