Fix Slack notifications: 'fields' is not a Block Kit block type - #474
Conversation
…#472) npm audit reported 5 high-severity advisories, all tracing to one root: a DoS/ReDoS bug in brace-expansion (GHSA-3jxr-9vmj-r5cp, GHSA-mh99-v99m-4gvg), reachable via eslint -> minimatch@3.1.5 -> brace-expansion@1.1.16. npm's only reported fix was eslint@10 (isSemVerMajor), which pulls in a new no-useless-assignment rule in @eslint/js's recommended config, flagging 15 pre-existing dead-store patterns across src/ -- too invasive for a security-only fix. Instead: add a root `overrides` entry (this repo already uses the same mechanism for esbuild/protobufjs/ws) forcing brace-expansion to the patched 5.0.8 line. Verified this reaches eslint's own tree (minimatch@3.1.5's brace-expansion now 5.0.8) while, as documented, leaving @earendil-works/pi-coding-agent's shrinkwrapped copy (5.0.6) untouched -- npm honors a shrinkwrap absolutely, so overrides can't reach it regardless. That shrinkwrapped copy is the only remaining high-severity finding, and it's exactly the node this gate's BUNDLED_ROOTS/ TOLERATED_BUNDLED_ADVISORIES allowlist already exists to tolerate -- added the second GHSA id to that allowlist (the gate is deliberately strict per-advisory-id, so a newly surfaced GHSA for an already-tolerated package still fails until consciously added). Verified: npm run lint unchanged (0 errors, same 4 pre-existing warnings -- confirms minimatch@3.1.5 works fine with the newer brace-expansion at runtime), npm run typecheck clean, gate logic simulated against real npm audit output now reports 0 blocking.
Slack notifications have never been deliverable. Both payload builders in
src/notifications/index.js emitted a block of `{ type: 'fields', fields }`,
but Block Kit has no `fields` block type — fields belong on a `section`
block. Slack rejects the entire attachment with:
400 invalid_attachments
Verified against a live Incoming Webhook: failing before this change,
delivering successfully after it.
The bogus type had propagated to every consumer, so each one is updated to
read fields off a section block while still tolerating the old shape, which
keeps any in-flight or persisted payloads converting correctly:
- channels/teams.js - facts extraction
- channels/discord.js - embed fields
- channels/text.js - plain-text flattening (covered by the existing
"slackPayloadToText renders blocks as readable
plain text" test, which caught this consumer)
Teams and Discord converters produce identical output to before the change.
tests/notifications-channels.test.js, harness-notifications.test.js and
notify-hook.test.js pass 21/21.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Strix Security ReviewNo security issues found. Updated for Reviewed by Strix |
|
Warning Review limit reached
Next review available in: 58 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Plus Run ID: ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (4)
📝 WalkthroughWalkthroughSlack notification formatters now emit valid section blocks containing fields, while Discord, Teams, and text conversions accept fields from both section and legacy fields blocks. ChangesSlack fields compatibility
Estimated code review effort: 2 (Simple) | ~10 minutes Suggested reviewers: Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
PR Summary by QodoFix Slack Block Kit payloads: move
AI Description
Diagram
High-Level Assessment
Files changed (4)
|
Code Review by Qodo
Context used✅ Compliance rules (platform):
300 rules✅ Skills:
|
![qodo-code-review[bot]](https://avatars.githubusercontent.com/in/484649?s=60&v=4){kind=small}
| // Fields ride on a section block; 'fields' is tolerated for older payloads. | ||
| if ((block.type === 'section' || block.type === 'fields') && block.fields) { | ||
| for (const f of block.fields) { |
There was a problem hiding this comment.
1. Legacy fields branch untested 📜 Skill insight ▣ Testability
A new backward-compatibility branch was added so Slack block parsing accepts both section blocks with fields and legacy block.type === 'fields', but the existing notification channel tests only exercise the section path. This leaves the legacy conversion path unverified and risks regressions for persisted/in-flight payloads shipping unnoticed.
Agent Prompt
## Issue description
Converters now accept both `section` blocks with `fields` and legacy `block.type === 'fields'`, but current tests only exercise the new `section` shape via `formatSlackStageMessage`, leaving the backward-compatibility branch untested.
## Issue Context
This PR explicitly aims to tolerate older persisted/in-flight Slack payloads; without a dedicated test that includes a legacy `{ type: 'fields', fields: [...] }` block, that compatibility guarantee can regress silently. Add a test that feeds a legacy Slack payload using `type: 'fields'` into each converter (e.g., `slackPayloadToText`, `convertSlackToDiscord`, `convertSlackToTeams`) and asserts the expected extracted fields / outputs.
## Fix Focus Areas
- tests/notifications-channels.test.js[19-42]
- src/notifications/channels/discord.js[20-33]
- src/notifications/channels/teams.js[17-40]
- src/notifications/channels/text.js[21-34]
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
Qodo flagged four issues on the eslint/brace-expansion security fix: - no test coverage for the TOLERATED_BUNDLED_ADVISORIES allowlist change - brace-expansion@5.0.8 declares engines.node "20 || >=22", excluding the Node 18 CI lane - the ">=5.0.8" override is unbounded and can float to a future major - the script comment mis-attributed the version bump to an eslint 9->10 bump that never happened (the real mechanism is the root override) Fix: bound the override to an exact "5.0.8" pin; export the pure tolerate/block decision logic from security-audit.mjs (evaluateAdvisories, isFullyBundled, advisoryIds) behind a main-guard so it's unit-testable without shelling out to npm audit, and add tests/security-audit.test.js covering the tolerate/block/allowlist-miss/severity-threshold paths, plus a pinned-allowlist regression test; correct the stale comment to document the real mechanism and the accepted Node 18 engines trade-off (no brace-expansion release fixes both GHSAs while keeping Node 18 support; it's a devDependency-only path and no engine-strict is set, so this doesn't fail CI in practice). Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
… path
CodeRabbit flagged that discord.js/teams.js/text.js all gained a branch
tolerating legacy block.type === 'fields' payloads (alongside the correct
section+fields shape), but only the section path was exercised in tests
— the compatibility guarantee could regress silently. Add a test that
feeds a legacy { type: 'fields', fields: [...] } block through all three
converters.
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
…to fix-slack-block-kit-fields-block-type
2 participants
Problem
Slack notifications have never been deliverable. Both payload builders in
src/notifications/index.js(formatSlackStageMessage,formatSlackTaskReportMessage) emit a block of:Block Kit has no
fieldsblock type — fields belong on asectionblock. Slack rejects the entire attachment:Because the failure is at the attachment level, no part of the message is delivered. Notifications are also fire-and-forget by design (
router.js), so this failed silently in normal operation.Reproduction
Against a live Incoming Webhook, with
RSTACK_SLACK_WEBHOOKset:After this change, same webhook:
Message confirmed delivered to the channel.
Fix
Producer now emits
type: 'section'withfields, which is the correct Block Kit shape.The bogus type had propagated to every consumer, so each is updated to read fields off a section block while still tolerating the old
'fields'shape — that keeps any persisted or in-flight payloads converting correctly:src/notifications/index.jschannels/teams.jschannels/discord.jschannels/text.jsVerification
tests/notifications-channels.test.js,tests/harness-notifications.test.js,tests/notify-hook.test.js— 21/21 pass.slackPayloadToText renders blocks as readable plain texttest caughttext.js, a consumer missed on the first pass — worth noting the suite did its job here.Note for maintainers
Separately from this fix: nothing loads a project's
.envintoprocess.env.resolveChannelsreadsprocess.envand.rstack/notifications.jsononly, whilecore/harness/env-file.jsis used solely by the Hub UI to edit keys. Following thedocs/integrations/webhooks.mdguidance to putRSTACK_SLACK_WEBHOOKin.envtherefore does not enable the channel unless the variable is also exported into the real environment. Left out of scope here — happy to open a separate issue.🤖 Generated with Claude Code
Summary by CodeRabbit