Skip to content

feat(release): compact Slack QA section + @-mention flagged-PR authors#35815

Open
nollymar wants to merge 2 commits into
mainfrom
feat/release-slack-qa-mentions
Open

feat(release): compact Slack QA section + @-mention flagged-PR authors#35815
nollymar wants to merge 2 commits into
mainfrom
feat/release-slack-qa-mentions

Conversation

@nollymar
Copy link
Copy Markdown
Member

Summary

Follow-up to #35762. Two issues surfaced from the first real-release Slack notification:

  1. The QA section was too tall — Slack collapsed it behind a "Show more" so the per-PR detail was effectively hidden.
  2. The @author text wasn't real Slack mentions, so the people whose action was needed never got notified.

Approach

Slack message stays compact. Just counts + a cc line:

:rotating_light: *QA Coverage* — 14 PRs need review
  <run-url|failed QA: 1>  |  <run-url|missing QA: 12>  |  <run-url|Orphan PRs: 1>  |  <run-url|Not in the core repo: 0>

cc <@U05R06D9YSX> @adrianjm-dotCMS @dario-daza <@UCF6CLC6L> ... — please review your PRs in this release.
  • Each count is a link to the workflow run summary.
  • Authors of failed / missing / orphan / external PRs are mentioned. Mapped users (via .github/data/slack-mappings.json) get real <@USERID> mentions that actually notify. Unmapped users fall back to plain @login so they're still named.
  • The header escalates to :rotating_light: whenever any PR is in the failed bucket.

Detail moves to the workflow run summary. A new --format markdown produces a GitHub-flavored markdown report (tables per bucket, links to issues, label cells) written to \$GITHUB_STEP_SUMMARY. Reviewers land there by clicking any count in Slack.

New CLI flags on release-qa-status

--format markdown    GitHub-flavored markdown — for $GITHUB_STEP_SUMMARY
--mappings PATH      slack-mappings.json for GH → Slack ID resolution
--detail-url URL     link target for each count in the slack output

Workflow changes

The Report step in cicd_6-release.yml now runs the script twice:

  1. --format markdown → appended to \$GITHUB_STEP_SUMMARY
  2. --format slack --mappings ... --detail-url <run-url> → injected into SLACK_MESSAGE

Both calls are best-effort (continue-on-error: true); a failure leaves the announcement intact.

Validation

End-to-end against v26.05.22-01 with the real mappings:

  • Slack snippet renders 14 flagged PRs as 4 count links + 1 cc line — well within Slack's inline-display limit.
  • Mapped users (dsilvam, hassandotcms, fmontes, ...) become real Slack mentions.
  • Unmapped users (adrianjm-dotCMS, dario-daza, ihoffmann-dot) fall back to plain @login.
  • Markdown summary renders correctly with bucket tables and inline labels.

Tests: 35/35 pass (previously 28/28). New cases cover detailUrl wrapping, mapped + unmapped + deduped + case-insensitive author resolution, exclusion of passed-bucket authors from the cc, and the markdown format.

Test plan

  • Manual workflow_dispatch run with notify_slack=true against a release
  • Confirm Slack message renders without truncation and that mentions notify the mapped users
  • Confirm workflow run summary contains the full markdown report
  • (Out of scope here) update slack-mappings.json for any newly-contributing authors so they get real mentions next time

🤖 Generated with Claude Code

The previous detailed Slack section grew too tall for the channel UI to
display inline — Slack auto-collapses messages over ~4000 chars, so the
list of flagged PRs ended up hidden behind a "Show more" link. The plain
`@author` text also didn't trigger Slack notifications, so the people
whose action was needed never knew.

Redesign:

  • The Slack snippet is now just the counts (each linked to the
    workflow run summary) and a cc line that mentions the authors of
    flagged PRs.

  • Mentions are real Slack `<@userid>` mentions when the GitHub login
    is in `.github/data/slack-mappings.json`. Unmapped users fall back
    to plain `@login` so the message still names them.

  • The full detailed report (PR-by-PR tables) is written to
    `$GITHUB_STEP_SUMMARY` via a new `--format markdown` so reviewers can
    drill in from the workflow run page linked by the counts.

New CLI flags on release-qa-status:
  --format markdown   GitHub-flavored markdown report
  --mappings PATH     slack-mappings.json for GH→Slack ID resolution
  --detail-url URL    link target for each count in the slack output

Tests: 35/35 pass. New cases cover detailUrl wrapping, the cc line with
mapped + unmapped + deduped + case-insensitive matching, exclusion of
passed-bucket authors from the cc, and the markdown format.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
@nollymar nollymar requested a review from a team as a code owner May 22, 2026 20:30
@github-actions github-actions Bot added the Area : CI/CD PR changes GitHub Actions/workflows label May 22, 2026
@claude
Copy link
Copy Markdown
Contributor

claude Bot commented May 22, 2026

Claude finished @nollymar's task in 1m 39s —— View job


Code review

Solid PR overall — well-factored, well-tested, and the split between compact Slack + detailed markdown is the right call. A few things to consider:

1) cc line dedup is case-sensitive but GitHub logins aren't

format.ts:279-283

const authors = new Set<string>();
for (const pr of report.failed) authors.add(pr.author);

GitHub logins are case-insensitive (Alice and alice are the same user), so if one flagged PR records the author as Hassandotcms and another as hassandotcms, both end up in the set. The mapping lookup at line 296 is case-insensitive (login.toLowerCase()), so the same user would even resolve to the same Slack ID and get <@U…> printed twice in the cc line. Your test at line 140 only covers same-case dedup.

Fix: dedup on the lowercased login (and either render with the lowercased form or keep a lowercase → originalCase map for display).

2) Slack @login fallback does not actually notify

The PR description says "Unmapped users fall back to plain @login so they're still named." Worth being explicit: in Slack mrkdwn, plain @login is just text — it does not notify the user (and won't even auto-link unless that string happens to match a real Slack username, in which case you might notify the wrong person). The current behavior is fine for visibility, but the description framing reads like a soft notification, which it isn't. Consider noting this in the PR body, or — better — failing the build / posting a ::warning:: when any flagged author is unmapped, so the gap gets closed proactively rather than discovered after the fact.

3) Heredoc delimiter collision risk in workflow

cicd_6-release.yml:272-276

echo 'qa_warning<<__RELEASE_QA_WARNING_EOF__'echo '__RELEASE_QA_WARNING_EOF__'

Vanishingly unlikely with this content, but GitHub's recommended pattern for arbitrary multi-line outputs is to pick a UUID-style delimiter (e.g. delim=$(uuidgen)) precisely so PR titles / author logins can never collide. Easy to harden if you want belt-and-suspenders.

4) Markdown PR-title escaping covers | only

format.ts:115-117

mdEscape only escapes pipes. PR titles in this repo regularly contain backticks (``) and brackets ([xyz]). Inside a table cell, an unbalanced backtick can swallow the rest of the row, and […](…) patterns can render as a link. Worth escaping at minimum backticks; brackets are a softer concern.

5) unlinked bucket's "Details" column is misleading

format.ts:136-140

For unlinked PRs the fallback shows pr.labels — i.e. labels on the PR, not on a linked issue (since there is none). Cell heading is just "Details" so this isn't wrong, but a reviewer reading the table next to the failed/missing tables (which show issue labels) may assume the same. Either rename the heading to "PR labels" for those buckets or drop the cell entirely for unlinked.

6) Author sort is locale-naive

Array.from(authors).sort() does a code-point sort, so Alice comes before bob. Not a bug, just slightly odd ordering in the cc line. sort((a,b) => a.toLowerCase().localeCompare(b.toLowerCase())) would be more predictable.


Nothing here is blocking — items 1 and 2 are the ones I'd actually do something about; the rest are nice-to-haves.
· Branch

Add stable HTML anchors before each populated bucket heading in the
markdown step-summary, and have the Slack count cells link to those
anchors instead of all pointing at the top of the run page.

Auto-generated heading slugs include the count (e.g. "qa-failed-1") so
they'd change every release; using explicit <a id="..."></a> markers
keeps the anchors stable.

Anchors: qa-failed | qa-missing | qa-orphan | qa-external. Empty
buckets emit no anchor (the heading is also omitted), so a healthy
release degrades cleanly to the all-clean summary message.

Tests: 37/37 pass.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

AI: Safe To Rollback Area : CI/CD PR changes GitHub Actions/workflows

Projects

Status: No status

Development

Successfully merging this pull request may close these issues.

2 participants