Skip to content

fix(ci): use pull_request_target for Slack review notifications#3731

Merged
aterga merged 4 commits intomainfrom
claude/sweet-franklin
Apr 7, 2026
Merged

fix(ci): use pull_request_target for Slack review notifications#3731
aterga merged 4 commits intomainfrom
claude/sweet-franklin

Conversation

@aterga
Copy link
Copy Markdown
Collaborator

@aterga aterga commented Apr 2, 2026

Changes

  • Changed workflow trigger from pull_request to pull_request_target in PR review notification workflow

Rationale

Using pull_request_target instead of pull_request allows the workflow to have proper access to repository secrets and permissions when handling review requests, which is necessary for sending Slack notifications reliably.

The notify-slack job fails on fork PRs because the `pull_request` event
does not expose secrets. Switching to `pull_request_target` gives the
workflow access to `SLACK_PRIVATE_IDENTITY_WEBHOOK_URL` while remaining
secure — checkout targets the base branch and no fork code is executed.

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
@aterga aterga requested a review from a team as a code owner April 2, 2026 17:49
Copilot AI review requested due to automatic review settings April 2, 2026 17:49
Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

Updates the PR review-request Slack notification workflow to use pull_request_target so it can access repository secrets (Slack webhook) when a review is requested.

Changes:

  • Switched workflow trigger from pull_request to pull_request_target for review_requested events.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

…ut, and author guard

Address PR review feedback:
- Add `permissions: contents: read` to restrict GITHUB_TOKEN scope
- Explicitly check out the base SHA with persist-credentials: false
- Gate the job on trusted author_association values to prevent
  notification spam from untrusted fork PRs

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

Copilot reviewed 1 out of 1 changed files in this pull request and generated 2 comments.


💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

aterga and others added 2 commits April 3, 2026 12:48
External contributors who previously had a commit merged still get
CONTRIBUTOR association, which is too broad for gating access to the
private Slack webhook. Restrict to MEMBER, OWNER, and COLLABORATOR.

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Replace fromJSON/contains with straightforward == checks for
readability and maintainability.

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
@aterga aterga requested a review from sea-snake April 7, 2026 14:49
@aterga aterga added this pull request to the merge queue Apr 7, 2026
Merged via the queue into main with commit 9bf1df9 Apr 7, 2026
46 checks passed
@aterga aterga deleted the claude/sweet-franklin branch April 7, 2026 15:39
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.

3 participants