Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
220 changes: 220 additions & 0 deletions .github/workflows/github-slack-notifications.yml
Original file line number Diff line number Diff line change
@@ -0,0 +1,220 @@
name: GitHub Slack Notifications

# Central GitHub -> Slack integration point. Two triggers feed one shared
# Slack workflow (via SLACK_WEBHOOK_URL), which branches on event_type:
# - issues opened -> notify oncall of a new issue
# - comments on closed PRs -> redirect the commenter to open an issue,
# and notify oncall (closed-PR comments are
# otherwise easy to miss)
# Every payload sends the same key set so the Slack workflow can branch
# reliably; fields that don't apply to an event are sent empty.

on:
issues:
types: [opened]
issue_comment:
types: [created]

jobs:
notify-issue-opened:
if: github.event_name == 'issues'
runs-on: ubuntu-latest
permissions: {}
steps:
- name: Send issue details to Slack
# Attacker-controlled fields are passed through env: rather than
# interpolated into the YAML payload, to prevent workflow injection.
# For issue_opened, the issue_* fields carry the data and the
# pr_*/comment_* fields are empty.
env:
REPOSITORY: ${{ github.repository }}
CREATED_AT: ${{ github.event.issue.created_at }}
ISSUE_NUMBER: ${{ github.event.issue.number }}
ISSUE_TITLE: ${{ github.event.issue.title }}
ISSUE_URL: ${{ github.event.issue.html_url }}
ISSUE_AUTHOR: ${{ github.event.issue.user.login }}
ISSUE_BODY: ${{ github.event.issue.body }}
LABELS: ${{ join(github.event.issue.labels.*.name, ', ') }}
uses: slackapi/slack-github-action@v2.1.1
with:
webhook: ${{ secrets.SLACK_WEBHOOK_URL }}
webhook-type: webhook-trigger
payload: |
event_type: "issue_opened"
repository: "${{ env.REPOSITORY }}"
created_at: "${{ env.CREATED_AT }}"
issue_number: "${{ env.ISSUE_NUMBER }}"
issue_title: ${{ toJSON(env.ISSUE_TITLE) }}
issue_url: "${{ env.ISSUE_URL }}"
issue_author: "${{ env.ISSUE_AUTHOR }}"
issue_body: ${{ toJSON(env.ISSUE_BODY) }}
labels: ${{ toJSON(env.LABELS) }}
pr_number: ""
pr_title: ""
pr_url: ""
pr_author: ""
pr_state: ""
pr_closed_at: ""
pr_merged_at: ""
comment_id: ""
comment_url: ""
comment_author: ""
comment_body: ""

closed-pr-comment-redirect:
# Only fire on comments left on PRs (issue_comment fires for issues too)
# that are already closed, and skip comments left by bots.
if: >-
github.event_name == 'issue_comment' && github.event.issue.pull_request != null && github.event.issue.state ==
'closed' && github.event.comment.user.type != 'Bot'
runs-on: ubuntu-latest
permissions:
pull-requests: write
issues: read
# Serialize per-PR so the marker-comment dedup is race-free (otherwise two
# rapid-fire comments could both see "no marker" and both post a redirect).
# cancel-in-progress is false so the second run still executes after the
# first finishes -- we want to evaluate dedup against the just-posted marker.
concurrency:
group: closed-pr-comment-${{ github.event.issue.number }}
cancel-in-progress: false
steps:
- name: Check commenter permission
id: perm
uses: actions/github-script@v9
with:
script: |
// External users on private repos can 404 here; treat any
// failure as "not a maintainer" so the redirect still fires.
let permission = 'none';
try {
const { data } = await github.rest.repos.getCollaboratorPermissionLevel({
owner: context.repo.owner,
repo: context.repo.repo,
username: context.payload.comment.user.login,
});
permission = data.permission;
} catch (err) {
core.info(`Permission lookup failed for ${context.payload.comment.user.login}: ${err.message}. Treating as non-maintainer.`);
}
const skip = ['admin', 'maintain', 'write'].includes(permission);
core.setOutput('skip', String(skip));
core.info(`Commenter ${context.payload.comment.user.login} permission=${permission} skip=${skip}`);

- name: Check for existing redirect comment
id: existing
if: steps.perm.outputs.skip != 'true'
uses: actions/github-script@v9
with:
script: |
// Marker we embed in our reply so we don't double-post on the same PR.
const marker = '<!-- closed-pr-comment-redirect -->';
const comments = await github.paginate(
github.rest.issues.listComments,
{
owner: context.repo.owner,
repo: context.repo.repo,
issue_number: context.payload.issue.number,
per_page: 100,
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.

This may fail on PRs with > 100 comments, but I think that's an unlikely edge case.

}
);
const alreadyPosted = comments.some(c => c.body && c.body.includes(marker));
core.setOutput('already_posted', String(alreadyPosted));

- name: Post redirect comment
if: steps.perm.outputs.skip != 'true' && steps.existing.outputs.already_posted != 'true'
# The Slack notification is the load-bearing part of this job. If
# posting the bot reply fails (rate limit, transient error), don't
# block the Slack notification.
continue-on-error: true
uses: actions/github-script@v9
env:
# Repos with issue templates use /issues/new/choose; repos without
# templates should change this to /issues/new.
ISSUES_NEW_URL: https://github.com/${{ github.repository }}/issues/new/choose
with:
script: |
const commenter = context.payload.comment.user.login;
const issuesNewUrl = process.env.ISSUES_NEW_URL;
const body = [
'<!-- closed-pr-comment-redirect -->',
'',
`Thanks for the report, @${commenter} — feedback like this is exactly`,
"how we catch the things we missed. Because this PR is already",
"closed, the team won't see follow-up comments here.",
'',
'Would you mind opening a new issue so we can track it properly?',
issuesNewUrl,
'',
'If this is a security issue, please report it privately via',
'https://aws.amazon.com/security/vulnerability-reporting/ instead',
'of a public issue.',
].join('\n');

await github.rest.issues.createComment({
owner: context.repo.owner,
repo: context.repo.repo,
issue_number: context.payload.issue.number,
body,
});

- name: Compute PR state
id: pr_state
if: steps.perm.outputs.skip != 'true'
uses: actions/github-script@v9
with:
script: |
// PRs surface as `issue` events; merged_at is null when closed-not-merged.
const mergedAt = context.payload.issue.pull_request &&
context.payload.issue.pull_request.merged_at;
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Slack notification is not deduped per PR/commenter, so oncall will be paged on every external comment.

The Post redirect comment step is gated by steps.existing.outputs.already_posted != 'true' (correct — we don't want to spam the PR with redirect replies), but Notify Slack is gated only on steps.perm.outputs.skip != 'true'. The result is:

  • A back-and-forth thread from one external user on a closed PR pages oncall on every comment.
  • A single user spamming a closed PR with N comments produces N pages.
  • Once the redirect comment has been posted, subsequent comments still page (despite the redirect having explicitly told them to open an issue).

Given this workflow exists specifically to close a detection gap, some over-paging is probably acceptable, but unbounded paging from a single thread is not. Options:

  1. Also gate Notify Slack on steps.existing.outputs.already_posted != 'true' — pages once per (PR, external commenter) cohort, but risks missing genuinely new info posted after the redirect.
  2. Keep the current behavior on the GitHub side and dedup on the Slack workflow side (e.g., suppress alerts where comment_author already alerted on the same pr_number within N hours). This is mentioned as a side-channel change, so the hook is already there.
  3. Track per-commenter dedup with a second marker comment scoped to the commenter login (similar to how existing works, but keyed on comment.user.login).

Whichever you pick, please document the chosen behavior in the comment block above this step — right now the comment says "Slack alert is the load-bearing part" but doesn't address repeat-comment volume.

core.setOutput('state', mergedAt ? 'merged' : 'closed');

- name: Notify Slack
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.

is it possible to use https://github.com/aws/agentcore-cli/blob/main/.github/workflows/slack-issue-notification.yml, as a central place to control github --> slack integration? My impression is that we have some logic here and there that rely on eachother.

# Notify oncall only on the FIRST external comment per PR (gated by
# already_posted). Subsequent comments on the same PR don't notify --
# the redirect comment has already directed the commenter to open an
# issue, and issues notify oncall via the issue path. This bounds
# notification volume regardless of how chatty a thread becomes.
if: steps.perm.outputs.skip != 'true' && steps.existing.outputs.already_posted != 'true'
# Attacker-controlled fields are passed through env: rather than
# interpolated into the YAML payload, to prevent workflow injection.
# For closed-PR comments, the issue_* fields are empty (this isn't
# an issue) and the pr_*/comment_* fields carry the real data.
env:
REPOSITORY: ${{ github.repository }}
CREATED_AT: ${{ github.event.comment.created_at }}
PR_NUMBER: ${{ github.event.issue.number }}
PR_TITLE: ${{ github.event.issue.title }}
PR_URL: ${{ github.event.issue.html_url }}
PR_AUTHOR: ${{ github.event.issue.user.login }}
PR_CLOSED_AT: ${{ github.event.issue.closed_at }}
PR_MERGED_AT: ${{ github.event.issue.pull_request.merged_at }}
COMMENT_ID: ${{ github.event.comment.id }}
COMMENT_URL: ${{ github.event.comment.html_url }}
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

pr_state env var will be empty in the payload because of step ordering / env: evaluation.

The env: block on this step references ${{ steps.pr_state.outputs.state }}, which is fine — that step runs before this one. But please double-check by running the workflow once: in some env: evaluation paths, steps.<id>.outputs.<x> is only available within with:/run: and not in step-level env:. If it does come through empty, the fix is either:

  1. Inline the expression directly in the payload: pr_state: "${{ steps.pr_state.outputs.state }}" (safe — values are merged/closed, not user input).
  2. Promote pr_state to a job-level output and reference it via needs/jobs (overkill for a single job).

Not a blocker if you've verified it works end-to-end on a test repo, but worth confirming since the manual test plan is post-merge.

COMMENT_AUTHOR: ${{ github.event.comment.user.login }}
COMMENT_BODY: ${{ github.event.comment.body }}
uses: slackapi/slack-github-action@v2.1.1
with:
webhook: ${{ secrets.SLACK_WEBHOOK_URL }}
webhook-type: webhook-trigger
payload: |
event_type: "closed_pr_comment"
repository: "${{ env.REPOSITORY }}"
created_at: "${{ env.CREATED_AT }}"
issue_number: ""
issue_title: ""
issue_url: ""
issue_author: ""
issue_body: ""
labels: ""
pr_number: "${{ env.PR_NUMBER }}"
pr_title: ${{ toJSON(env.PR_TITLE) }}
pr_url: "${{ env.PR_URL }}"
pr_author: "${{ env.PR_AUTHOR }}"
pr_state: "${{ steps.pr_state.outputs.state }}"
pr_closed_at: "${{ env.PR_CLOSED_AT }}"
pr_merged_at: "${{ env.PR_MERGED_AT }}"
comment_id: "${{ env.COMMENT_ID }}"
comment_url: "${{ env.COMMENT_URL }}"
comment_author: "${{ env.COMMENT_AUTHOR }}"
comment_body: ${{ toJSON(env.COMMENT_BODY) }}
24 changes: 0 additions & 24 deletions .github/workflows/slack-issue-notification.yml

This file was deleted.

Loading