fix(ci): don't fail benchmark on fork PRs when an alert fires#1464
Open
wakame1367 wants to merge 1 commit into
Open
fix(ci): don't fail benchmark on fork PRs when an alert fires#1464wakame1367 wants to merge 1 commit into
wakame1367 wants to merge 1 commit into
Conversation
`fail-on-alert: false` is set, so a performance alert is not supposed to fail the job. But with `comment-on-alert: true` the action tries to POST a PR comment, and pull requests from a fork only get a read-only GITHUB_TOKEN, so the write fails with "Resource not accessible by integration" and takes the whole job down. Only enable the alert comment when the pull request does not come from a fork. Behaviour on push events and on same-repo pull requests is unchanged. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What
benchmark.ymlsetsfail-on-alert: false, so a performance alert is not meant to fail the job. But it also setscomment-on-alert: true, and pull requests from a fork only get a read-onlyGITHUB_TOKEN— so posting the alert comment fails and takes the whole job down:The result is that an external contributor gets a red ✗ with a confusing permissions error on a PR that didn't change any runtime code. It's easy to hit because at this timing granularity a
0.03s→0.04sjitter already exceeds the130%threshold.I ran into this on #1463 (a docs-only change). Note it isn't every fork PR — only the ones where an alert happens to fire, which is why e.g. #1448 was fine.
Change
Enable the alert comment only when the pull request does not come from a fork:
Behaviour is unchanged everywhere it currently works:
...head.repo.forkcomment-on-alertpush(main / dev / tags)nulltruefalsetruetruefalseThe benchmark itself still runs and the alert is still surfaced in the job log — only the unauthorized comment attempt is skipped.
Verification
pre-commit run --files .github/workflows/benchmark.ymlpasses (this includes the repo'szizmor --min-severity mediumgate).zizmor --persona pedanticreports exactly the same findings before and after this change (1 informational, 1 low, 1 medium — all pre-existing onmain), so no new findings are introduced.pull_requestruns use the workflow definition from the PR head, this PR exercises the change against itself: the benchmark job on this (fork) PR logscomment-on-alert: false, confirming the expression resolves as intended. No alert happened to fire in that run, so the alert path itself wasn't hit here — but with commenting disabled,handleAlertno longer attempts the unauthorized write that was failing the job.🤖 Generated with Claude Code