Skip to content

ci: add concurrency group to pr-review-trigger to prevent duplicate reviews#7079

Open
derekmisler wants to merge 2 commits into
docker:masterfrom
derekmisler:fix/pr-review-trigger-concurrency
Open

ci: add concurrency group to pr-review-trigger to prevent duplicate reviews#7079
derekmisler wants to merge 2 commits into
docker:masterfrom
derekmisler:fix/pr-review-trigger-concurrency

Conversation

@derekmisler

Copy link
Copy Markdown
Contributor

Adds a concurrency group keyed by PR number to pr-review-trigger.yml to prevent
triplicate reviews when simultaneous pull_request events fire for the same fork PR
(e.g., multiple review_requested events when several reviewers are added at once).

Also skips Bot-type senders (Dependabot, Renovate) early to save Actions minutes.

@docker-agent docker-agent left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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-commenter

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.

📢 Thoughts on this report? Let us know!

@derekmisler derekmisler force-pushed the fix/pr-review-trigger-concurrency branch from c2c6ea1 to 76b3700 Compare June 30, 2026 16:20
…eviews

Signed-off-by: Derek Misler <derek.misler@docker.com>
@derekmisler derekmisler force-pushed the fix/pr-review-trigger-concurrency branch from 76b3700 to 20976e4 Compare June 30, 2026 16:26
@derekmisler derekmisler marked this pull request as ready for review June 30, 2026 16:30

@docker-agent docker-agent left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 }}

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[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 }}

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Comment thread .github/workflows/pr-review-trigger.yml Outdated
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'

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[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.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@thaJeztah thaJeztah added this to the 29.7.0 milestone Jun 30, 2026
Comment thread .github/workflows/pr-review-trigger.yml Outdated

jobs:
save-context:
# Only run on fork PRs; skip GitHub App bot accounts (Dependabot, Renovate, etc.) early.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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)

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants