ci: add concurrency group to pr-review-trigger to prevent duplicate reviews#7079
ci: add concurrency group to pr-review-trigger to prevent duplicate reviews#7079derekmisler wants to merge 2 commits into
Conversation
docker-agent
left a comment
There was a problem hiding this comment.
Assessment: 🟢 APPROVE
The concurrency group addition and bot-sender skip logic are both correct. The github.event.pull_request.number expression is the right key for deduplicating concurrent runs on the same PR, and cancel-in-progress: true is appropriate here since only the latest trigger needs to run. The bot-skip condition (github.event.sender.type == 'Bot') is a valid way to short-circuit early for Dependabot/Renovate and will not affect human-authored PRs.
No issues found in the changed code.
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
c2c6ea1 to
76b3700
Compare
…eviews Signed-off-by: Derek Misler <derek.misler@docker.com>
76b3700 to
20976e4
Compare
docker-agent
left a comment
There was a problem hiding this comment.
Assessment: 🟡 NEEDS ATTENTION
Two medium-severity findings in the newly added concurrency group and job guard. Both relate to how github.event.pull_request resolves for pull_request_review_comment events. GitHub's webhook docs indicate the payload does include a pull_request field for that event type — but the current expressions have no fallback if the field is absent, which could cause cross-PR cancellations or silently skipped comment-triggered jobs. See inline comments for details and suggested fixes.
| # 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.
[MEDIUM] Concurrency group key may resolve to empty string for pull_request_review_comment events
The group key is pr-review-trigger-${{ github.event.pull_request.number }}. The workflow also triggers on pull_request_review_comment events. Whether github.event.pull_request is populated for that event type is a critical question: GitHub's webhook docs show a pull_request field in the pull_request_review_comment payload, which would mean the key resolves correctly. However, if for any reason the field is absent or empty at runtime, all comment-triggered runs would share the group key pr-review-trigger- and cancel-in-progress: true would cause them to cancel each other across different PRs.
Consider using a safer fallback expression to guarantee uniqueness:
group: pr-review-trigger-${{ github.event.pull_request.number || github.event.issue.number }}Or include the event name for an extra layer of scoping:
group: pr-review-trigger-${{ github.event_name }}-${{ github.event.pull_request.number }}There was a problem hiding this comment.
The pull_request_review_comment webhook payload includes a pull_request field with number populated per GitHub's documentation. The concurrency group key will always resolve to a valid PR number for both event types. The docker-agent APPROVE review (submitted first) confirmed the expressions are correct.
| 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.
[MEDIUM] if: guard may silently skip pull_request_review_comment events
The job condition is:
if: github.event.pull_request.head.repo.fork && github.event.sender.type != 'Bot'The run: step at line 34 explicitly handles pull_request_review_comment events (it writes context/comment.json), indicating intent to process them. However, if github.event.pull_request is ever absent or fork evaluates to a falsy value for comment events, the job silently skips — the comment context is never saved, and downstream review jobs would not fire.
As a defensive measure, consider making the fork check event-aware:
if: >-
(github.event.pull_request.head.repo.fork ||
github.event_name == 'pull_request_review_comment') &&
github.event.sender.type != 'Bot'This ensures comment-triggered runs proceed regardless of how the pull_request object resolves for that event type.
There was a problem hiding this comment.
The pull_request object is present in pull_request_review_comment payloads, so head.repo.fork resolves correctly for comment events too. The docker-agent APPROVE review confirmed no issues with these expressions.
There was a problem hiding this comment.
The developer is correct. Looking at the GitHub webhook documentation, the pull_request_review_comment event payload does include a full pull_request object with the head.repo.fork property. My concern was unfounded — the condition will evaluate correctly for both event types.
I appreciate the clarification, and I apologize for the noise. This was a false positive based on an incorrect assumption about the event payload structure.
|
|
||
| jobs: | ||
| save-context: | ||
| # Only run on fork PRs; skip GitHub App bot accounts (Dependabot, Renovate, etc.) early. |
There was a problem hiding this comment.
Is there an option to still review dependabot updates? They've been useful for some updates to review dependency changes (call our potentially dangerous and/or behavior changes in dependencies)
There was a problem hiding this comment.
Good point — removed. The bot filter has been dropped, so Dependabot PRs will continue to be reviewed as before. The if: is now just github.event.pull_request.head.repo.fork, keeping the fork-only scope without filtering by sender type.
The concurrency group (the main fix for duplicate reviews when multiple reviewers are added at once) is independent and still in place.
The sender.type != 'Bot' guard would have prevented reviews on Dependabot PRs. Per maintainer feedback, those reviews are useful for catching behavior changes in dependency updates. The concurrency group (the core fix for duplicate reviews) is independent of the bot filter and remains in place.
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.