ci: add concurrency group to pr-review-trigger to prevent duplicate reviews#25469
ci: add concurrency group to pr-review-trigger to prevent duplicate reviews#25469derekmisler wants to merge 1 commit into
Conversation
✅ Deploy Preview for docsdocker ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
docker-agent
left a comment
There was a problem hiding this comment.
Assessment: 🔴 CRITICAL
This PR introduces two functional bugs in the new block and job condition, both caused by using expressions that are only populated on events — not on the events this workflow also handles.
| # review_requested events. Without this group each event triggers a | ||
| # separate review via workflow_run, producing duplicate reviews. | ||
| concurrency: | ||
| group: pr-review-trigger-${{ github.event.pull_request.number }} |
There was a problem hiding this comment.
[HIGH] Concurrency group key is empty for pull_request_review_comment events, causing cross-PR cancellation
For pull_request_review_comment events, github.event.pull_request is absent from the webhook payload — the PR data is not included at the top level of that event type. This means github.event.pull_request.number resolves to an empty string, and the concurrency group key becomes the bare string pr-review-trigger- for every review comment event on every PR. With cancel-in-progress: true, any new pull_request_review_comment run will cancel any other in-flight comment-triggered run, regardless of which PR it belongs to.
To fix, use a conditional expression that works for both event types:
concurrency:
group: pr-review-trigger-${{ github.event.pull_request.number || github.event.issue.number }}
cancel-in-progress: trueOr more explicitly:
group: >-
pr-review-trigger-${{
github.event_name == 'pull_request_review_comment'
&& github.event.issue.number
|| github.event.pull_request.number
}}Note: for pull_request_review_comment events the PR number is available as github.event.issue.number.
There was a problem hiding this comment.
Per GitHub's webhook documentation, pull_request_review_comment payloads do include a nested pull_request object with number and head.repo.fork populated. The docker-agent second review (APPROVE, submitted later) confirmed the expressions resolve correctly for both event types.
| jobs: | ||
| save-context: | ||
| # Only run on fork PRs; skip GitHub App bot accounts (Dependabot, Renovate, etc.) early. | ||
| if: github.event.pull_request.head.repo.fork && github.event.sender.type != 'Bot' |
There was a problem hiding this comment.
[HIGH] if condition always evaluates to false for pull_request_review_comment events, silently skipping the job
The job condition github.event.pull_request.head.repo.fork relies on github.event.pull_request being populated. For pull_request_review_comment events this object is absent, so the expression resolves to an empty/falsy value and the entire save-context job is skipped silently for every review comment event. The pull_request_review_comment trigger on line 5-6 therefore has no effect — the workflow fires but immediately does nothing.
To handle both event shapes, the condition should distinguish between event types:
if: |
(github.event_name == 'pull_request_review_comment' ||
github.event.pull_request.head.repo.fork) &&
github.event.sender.type != 'Bot'For pull_request_review_comment events there is no head.repo.fork equivalent at the top level, so allowing those events through unconditionally (or checking a different signal) is the appropriate approach.
There was a problem hiding this comment.
Same as above — the pull_request object is present in pull_request_review_comment payloads. The later APPROVE review from docker-agent confirmed both expressions are sound.
There was a problem hiding this comment.
You're absolutely right — I apologize for the confusion. Looking at the GitHub webhook documentation more carefully, pull_request_review_comment events do include the full pull_request object in the payload, so github.event.pull_request.head.repo.fork is indeed accessible and will evaluate correctly.
My original concern was misplaced. The condition works as intended for both event types. Thank you for the correction — I should have verified the event payload structure more thoroughly before raising the issue.
docker-agent
left a comment
There was a problem hiding this comment.
Assessment: 🟢 APPROVE
The concurrency group addition is syntactically correct and uses the standard GitHub Actions deduplication pattern. The bot-skip condition (sender.type != 'Bot') correctly identifies Dependabot and Renovate. No high or medium severity issues found.
Adds a concurrency group keyed by PR number to
pr-review-trigger.ymlto preventtriplicate reviews when simultaneous
pull_requestevents fire for the same fork PR(e.g., multiple
review_requestedevents when several reviewers are added at once).Also skips Bot-type senders (Dependabot, Renovate) early to save Actions minutes.