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 1 commit 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! |
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.
|
oh! looks like your second commit is missing a DCO sign-off; can you add one? (possibly can be squashed as well) |
Add a concurrency group keyed on PR number to prevent duplicate reviews from simultaneous review_requested events. Remove the sender.type != 'Bot' guard so Dependabot PRs remain reviewable — per maintainer feedback, those reviews are useful for catching behavior changes in dependency updates. Signed-off-by: Derek Misler <derek.misler@docker.com>
ecd53d5 to
36c96ce
Compare
|
Done, @thaJeztah! Squashed both commits into one with the DCO sign-off included and force-pushed to the branch. |
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.