diff --git a/.github/workflows/a11y-bucket-notify.yml b/.github/workflows/a11y-bucket-notify.yml deleted file mode 100644 index 2d368e9232..0000000000 --- a/.github/workflows/a11y-bucket-notify.yml +++ /dev/null @@ -1,132 +0,0 @@ -# Security review notes: -# - The only event-derived strings used in payloads are PR title, URL, number, -# author, and label name. They are exposed via env: vars (PR_TITLE, etc.) -# and referenced as ${{ env.* }} in the YAML payload literal -- mirrors the -# sibling design-feedback-notify.yml workflow's pattern. -# - The author_association guard restricts Slack pings to MEMBER / -# COLLABORATOR PRs, matching the sibling workflow's policy of not pinging -# on external-contributor labels. -# - No shell `run:` interpolation of untrusted strings. -# - issues: write is required so the dedup step can create/update the hidden -# marker comment that records the last bucket pinged. Without this, removing -# and re-adding the same bucket label (e.g., via A11y-Audit-Ref trailer -# edit cycles) would fire a duplicate Slack ping. -name: A11y Bucket Slack Notification - -on: - pull_request: - types: [labeled] - -permissions: - contents: read - pull-requests: read - issues: write - -# Serialize runs per PR so the marker check is race-free across rapid label -# events on the same PR (e.g., a triage run that flips bucket-3 -> bucket-4 -# back-to-back). -concurrency: - group: a11y-bucket-notify-${{ github.event.pull_request.number }} - cancel-in-progress: false - -jobs: - notify: - if: >- - (github.event.label.name == 'a11y:bucket-3' || github.event.label.name == 'a11y:bucket-4') && - contains(fromJSON('["MEMBER","COLLABORATOR"]'), github.event.pull_request.author_association) - runs-on: ubuntu-latest - steps: - - name: Check ping history - id: dedup - uses: actions/github-script@v7 - env: - BUCKET: ${{ github.event.label.name }} - with: - script: | - const { owner, repo } = context.repo; - const issue_number = context.payload.pull_request.number; - const bucket = process.env.BUCKET; - const MARKER_PREFIX = '/); - if (match && match[1] === bucket) { - core.info(`Already pinged for ${bucket} on this PR; skipping duplicate Slack notification.`); - shouldSkip = true; - } - } - - core.setOutput('skip', shouldSkip ? 'true' : 'false'); - core.setOutput('existing_id', existing ? String(existing.id) : ''); - - - name: Notify Slack - if: steps.dedup.outputs.skip != 'true' - uses: slackapi/slack-github-action@45a88b9581bfab2566dc881e2cd66d334e621e2c # v3.0.3 - env: - PR_TITLE: ${{ github.event.pull_request.title }} - PR_URL: ${{ github.event.pull_request.html_url }} - PR_NUMBER: ${{ github.event.pull_request.number }} - PR_AUTHOR: ${{ github.event.pull_request.user.login }} - BUCKET: ${{ github.event.label.name }} - with: - method: chat.postMessage - token: ${{ secrets.SLACK_BOT_TOKEN }} - payload: | - channel: "${{ vars.DESIGN_FEEDBACK_SLACK_CHANNEL }}" - text: "A11y review needed (${{ env.BUCKET }}): ${{ env.PR_TITLE }}" - blocks: - - type: header - text: - type: plain_text - text: "Accessibility PR needs engineer review" - - type: section - text: - type: mrkdwn - text: "*<${{ env.PR_URL }}|#${{ env.PR_NUMBER }}: ${{ env.PR_TITLE }}>*" - - type: section - fields: - - type: mrkdwn - text: "*Bucket:*\n${{ env.BUCKET }}" - - type: mrkdwn - text: "*Author:*\n${{ env.PR_AUTHOR }}" - - type: mrkdwn - text: "*Repository:*\n${{ github.repository }}" - - type: actions - elements: - - type: button - text: - type: plain_text - text: "View PR" - style: primary - url: "${{ env.PR_URL }}" - - - name: Update ping marker - if: steps.dedup.outputs.skip != 'true' - uses: actions/github-script@v7 - env: - BUCKET: ${{ github.event.label.name }} - EXISTING_ID: ${{ steps.dedup.outputs.existing_id }} - with: - script: | - const { owner, repo } = context.repo; - const issue_number = context.payload.pull_request.number; - const body = `\nSlack notification dedup marker (DT-4048).`; - if (process.env.EXISTING_ID) { - await github.rest.issues.updateComment({ - owner, - repo, - comment_id: parseInt(process.env.EXISTING_ID, 10), - body, - }); - } else { - await github.rest.issues.createComment({ owner, repo, issue_number, body }); - } diff --git a/.github/workflows/a11y-pr-triage.yml b/.github/workflows/a11y-pr-triage.yml index a64dce4719..b34465d058 100644 --- a/.github/workflows/a11y-pr-triage.yml +++ b/.github/workflows/a11y-pr-triage.yml @@ -6,6 +6,9 @@ # event payload. # - All write operations (labels, comments) go through the github-script-bound # Octokit, never through shell. +# - The Slack notification step receives PR title / URL / author via step +# outputs that came from JS context, then re-exposed via env: vars. No shell +# interpolation of untrusted strings. name: A11y PR Triage on: @@ -37,8 +40,12 @@ jobs: - name: Install js-yaml run: npm install --no-save --no-audit --no-fund --silent js-yaml@^4 - - name: Triage and label + - name: Triage, label, and notify + id: triage uses: actions/github-script@v7 + env: + SLACK_BOT_TOKEN: ${{ secrets.SLACK_BOT_TOKEN }} + SLACK_CHANNEL: ${{ vars.DESIGN_FEEDBACK_SLACK_CHANNEL }} with: script: | const fs = require('fs'); @@ -149,6 +156,80 @@ jobs: await github.rest.issues.addLabels({ owner, repo, issue_number, labels: toAdd }); } + // Slack notification gating. The previous bucket-notify-via-labeled-event + // design didn't work because GITHUB_TOKEN-triggered events don't fire + // downstream workflows (GitHub's recursion-prevention rule). We inline + // the ping here and dedup via a hidden bot comment so trailer-edit + // cycles don't double-ping. + const PING_BUCKETS = ['a11y:bucket-3', 'a11y:bucket-4']; + const newlyAddedBucket = toAdd.find((l) => PING_BUCKETS.includes(l)); + + if (newlyAddedBucket) { + try { + const NOTIFY_MARKER_PREFIX = '/); + if (m) lastPinged = m[1]; + } + if (lastPinged === newlyAddedBucket) { + core.info(`Already pinged Slack for ${newlyAddedBucket}; skipping.`); + } else { + const newBody = `\nSlack notification dedup marker (DT-4048).`; + if (marker) { + await github.rest.issues.updateComment({ owner, repo, comment_id: marker.id, body: newBody }); + } else { + await github.rest.issues.createComment({ owner, repo, issue_number, body: newBody }); + } + // CONTRIBUTOR is included because GITHUB_TOKEN-issued lookups + // return CONTRIBUTOR for org members with private membership + // (whose membership visibility isn't visible to repo-scope + // tokens). All four values represent "authorized author." + const ALLOWED_ASSOC = ['MEMBER', 'COLLABORATOR', 'OWNER', 'CONTRIBUTOR']; + if (!ALLOWED_ASSOC.includes(pr.author_association)) { + core.info(`Skipping Slack ping: author_association ${pr.author_association} not in allowed list.`); + } else { + const slackPayload = { + channel: process.env.SLACK_CHANNEL, + text: `A11y review needed (${newlyAddedBucket}): ${pr.title}`, + blocks: [ + { type: 'header', text: { type: 'plain_text', text: 'Accessibility PR needs engineer review' } }, + { type: 'section', text: { type: 'mrkdwn', text: `*<${pr.html_url}|#${pr.number}: ${pr.title}>*` } }, + { type: 'section', fields: [ + { type: 'mrkdwn', text: `*Bucket:*\n${newlyAddedBucket}` }, + { type: 'mrkdwn', text: `*Author:*\n${(pr.user && pr.user.login) || ''}` }, + { type: 'mrkdwn', text: `*Repository:*\n${owner}/${repo}` }, + ] }, + { type: 'actions', elements: [ + { type: 'button', text: { type: 'plain_text', text: 'View PR' }, style: 'primary', url: pr.html_url }, + ] }, + ], + }; + const slackResp = await fetch('https://slack.com/api/chat.postMessage', { + method: 'POST', + headers: { + 'Content-Type': 'application/json; charset=utf-8', + 'Authorization': `Bearer ${process.env.SLACK_BOT_TOKEN}`, + }, + body: JSON.stringify(slackPayload), + }); + const slackResult = await slackResp.json(); + if (slackResult.ok) { + core.info(`Slack ping sent (${newlyAddedBucket}) to ${process.env.SLACK_CHANNEL}`); + } else { + core.warning(`Slack ping failed: ${slackResult.error || 'unknown'} (status ${slackResp.status})`); + } + } + } + } catch (err) { + core.warning(`A11y Slack section failed: ${err && err.message}`); + } + } + const failureLabels = ['a11y:needs-categorization', 'a11y:broken-ref', 'a11y:no-fix-doc']; const triggered = desiredArr.filter((l) => failureLabels.includes(l));