slack notification update - #1326
Conversation
Code ReviewReviewed for correctness only (docs content/style is handled by a separate workflow). The security posture of the new 1. 🔴 Blocker — labels applied with
|
| Template | Labels |
|---|---|
content_fix.yml |
documentation, fix |
contend_add.yml |
enhancement, documentation |
product_bug.yml |
bug, product |
site_fix.yml |
docusaurus, fix, site |
site_add.yml |
enhancement, site, docusaurus |
product_proposal.yml |
enhancement, product |
By the time the auto-labeler adds a product label, .issue.labels | length is ≥ 3, so skip=true on every run. Even with finding #1 fixed, issue notifications stay dead. A "have I already notified?" check needs real idempotency state — e.g. look for an existing bot marker comment, or gate on "the label that triggered this run is a product label and no other product label was already present" (github.event.label.name) rather than on total label count.
3. 🟠 Multi-label batches likely suppress the PR notification too
label-pr.yml adds all labels in a single gh pr edit --add-label A --add-label B call. GitHub emits one labeled event per label, and each payload carries the label set as of that event — for a batched add, that is generally the full post-add set. If so, label_count is ≥ 2 on every event and both the Slack message and the fork comment are skipped.
This is guaranteed to bite for at least one product: endpoint-policy-manager and policypak both map to @netwrix/endpointpolicymanager-docs (see #4), so any PolicyPak PR always gets exactly two labels. Any PR touching two products has the same problem.
(If GitHub instead delivers incrementally-growing label lists, exactly one event would have label_count == 1 and this specific case works — worth confirming empirically before relying on it.)
4. 🟠 The label→team map is not 1:1, so the reverse lookup over-labels
scripts/resolve-labels-for-teams.mjs walks label-codeowners.json and collects every label whose team matches. The mapping is many-to-one:
@netwrix/endpointpolicymanager-docs <- endpoint-policy-manager, policypak
So a PR under /docs/policypak/ gets both labels applied. Also note /docs/recoveryforactivedirectory/ → @netwrix/recoveryforactivedirectory-docs → reverse-maps only to the label identity-recovery, which is a confusing label for that path.
Consider making the mapping explicitly bidirectional (or adding a canonical-label marker) rather than inferring the reverse direction from a lossy forward map.
5. 🟠 PRs that resolve to no product label now get no notification at all
Previously the notify job resolved teams from CODEOWNERS and posted "PR created" even when TEAMS came back empty — the old comment said as much ("an empty result here just means no team mention — it does not skip the notification itself"). Now, no label means no labeled event, which means no workflow run and no message.
Two ways this happens:
- Paths outside CODEOWNERS. This very PR is an example — it only touches
.github/andscripts/, which match no CODEOWNERS rule, so it would produce zero Slack notification. - Teams with no label.
@netwrix/training-docs,@netwrix/platgovnetsuiteflashlight-docs, and@netwrix/platgovsalesforceflashlight-docsown 13 CODEOWNERS lines between them but have no entry inlabel-codeowners.json. (Theirteam-slack-map.jsonentries are empty arrays, so they'd get no @-mention either way — but under the old flow the channel at least saw the PR.) These areas will also never receive a product label, which is a labeling gap independent of Slack.
6. 🟡 gh pr view --json files truncates at 100 files
FILES=$(gh pr view ... --json files --jq '.files[].path')gh pr view --json files is capped at 100 files. This was already true in the old notify workflow, but the consequence is bigger now: truncation silently drops product labels, and labels drive both the PR's visible metadata and whether any notification fires at all. Docs PRs in this repo routinely exceed 100 changed files. Use gh api --paginate repos/${{ github.repository }}/pulls/N/files --jq '.[].path' instead.
7. 🟡 "PR created" is now sent on any first label, not on PR creation
The message text is still "PR created: ..." but the trigger is labeled. Two consequences:
- If a PR opens with zero labels and only gets its first label later (e.g. a
synchronizethat finally touches a product directory),label_count == 1holds and "PR created" is posted hours after the fact. - A human manually adding the first label also fires "PR created".
Also, label-pr.yml only ever adds labels — a PR that later drops a product's files keeps the stale label, so team mentions can be wrong on subsequent events.
8. 🟡 cancel-in-progress: true on label-pr.yml can leave a PR unlabeled
concurrency:
group: label-pr-${{ github.event.pull_request.number }}
cancel-in-progress: trueRapid pushes cancel the in-flight labeling run. Usually self-healing since the next synchronize re-runs the whole computation, but a cancellation on the final push leaves the PR unlabeled — and per #5 that now means no notification ever. cancel-in-progress: false (matching the two notify workflows) is safer for a job whose side effect is the trigger for something else.
Nits
resolve-labels-for-teams.mjs: the!labels.includes(label)dedupe is dead code —Object.entries()keys are already unique.label-pr.ymldoes a full checkout; the sibling workflows usesparse-checkout: [.github, scripts], which is all this job needs.
Confirmed fine
- Security of the new
pull_request_targetworkflow. Checkout is pinned togithub.event.pull_request.base.shawith no head/merge ref, actions are SHA-pinned, and only PR metadata is read via the API. Attacker-controlled file paths are passed tonodeas a properly quoted"${FILE_ARRAY[@]}"array, andgithub.event.pull_request.numberis numeric — no injection vector. The inline SAFETY comments are a good addition. sync-dev-to-main.yml. Teams removal is complete: no danglingTEAMS_WEBHOOK_URLreferences, and the four remaining Slack steps (success / conflict / skip / failure) have mutually exclusive conditions with no duplicates.slack-notify-issue.ymlskip plumbing. Theskip=trueearly exits are correctly consumed byif: steps.codeowners.outputs.skip != 'true'on the send step, and the dedupe rewrite ([[ ",$TEAMS," != *",$team,"* ]]) fixes a real substring-collision bug in the oldgrep -qFapproach. The[ -z "$label" ] && continueguard is a good catch for the empty-LABELScase.
No description provided.