From 3865694cf9447df215eceb9d4b1fd274a3117432 Mon Sep 17 00:00:00 2001 From: Benjamin Ross Date: Wed, 22 Jul 2026 03:54:43 -0700 Subject: [PATCH] fix(slack): surface Block Kit content in fetched thread context MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Bot-posted alerts (Honeycomb, PagerDuty, Datadog, GitHub bot, etc.) carry their actionable content — section text, button URLs — in Block Kit blocks, while the plain text field holds only the alert title. _fetch_thread_context and _fetch_thread_parent_text only read msg.get('text'), so that content never reached the agent. Add a _render_message_text helper that merges top-level text with readable block content, section/header/context text, actionable URLs, and (folded in from #61261 during conflict resolution) legacy attachment fields, and use it for thread-context and parent-text rendering. Salvaged from #29541. --- plugins/platforms/slack/adapter.py | 121 ++++++++++++---- tests/gateway/test_slack_approval_buttons.py | 140 ++++++++++++++++++- 2 files changed, 233 insertions(+), 28 deletions(-) diff --git a/plugins/platforms/slack/adapter.py b/plugins/platforms/slack/adapter.py index 578b256714e2b..b08135c3b1dcc 100644 --- a/plugins/platforms/slack/adapter.py +++ b/plugins/platforms/slack/adapter.py @@ -373,6 +373,47 @@ def _serialize_slack_blocks_for_agent(blocks: list, max_chars: int = 6000) -> st return f"[Slack Block Kit payload for this message]\n```json\n{payload}\n```" +def _extract_urls_from_slack_blocks(blocks: list) -> list[str]: + """Walk a Block Kit ``blocks`` tree and return URLs found on any element. + + Returns URLs preserving discovery order with duplicates removed. Used to + surface the actionable links (``View graph``, ``View incident``, etc.) + embedded in bot-posted alerts so an agent reading the thread can fetch + or click them. The companion serializer + :func:`_serialize_slack_blocks_for_agent` deliberately strips ``url`` to + keep the JSON view compact and to avoid exposing arbitrary URLs through + the generic payload dump; this helper is the targeted opt-in for + use sites where URLs are the whole point of the message. + """ + if not blocks: + return [] + + found: list[str] = [] + seen: set[str] = set() + + def _maybe_add(value: Any) -> None: + if isinstance(value, str) and value.startswith(("http://", "https://")): + if value not in seen: + seen.add(value) + found.append(value) + + def _walk(node: Any) -> None: + if isinstance(node, dict): + # The common URL-bearing keys across Block Kit (buttons, link + # elements in rich_text, image accessories, etc.). + for key in ("url", "image_url", "external_url"): + if key in node: + _maybe_add(node[key]) + for value in node.values(): + _walk(value) + elif isinstance(node, list): + for item in node: + _walk(item) + + _walk(blocks) + return found + + def _apply_slack_proxy(client: Any, proxy_url: Optional[str]) -> None: """Apply a resolved proxy to a Slack SDK client or clear it explicitly.""" if hasattr(client, "proxy"): @@ -4400,6 +4441,57 @@ class SlackAdapter(BasePlatformAdapter): # ----- Thread context fetching ----- + @staticmethod + def _render_message_text(msg: dict, bot_uid: str = "") -> str: + """Return bounded display text for a Slack message, surfacing Block Kit content. + + Starts with ``text``, strips bot mentions, then appends rich-text + content and actionable URLs from ``blocks`` when present. Unlike + :func:`_serialize_slack_blocks_for_agent` (which can emit up to + 6 000 chars of JSON per message), this helper produces only the + readable text and URL list needed by thread-context and parent- + text rendering — bounded by what the blocks actually contain, + not a JSON dump. + """ + msg_text = (msg.get("text") or "").strip() + if bot_uid: + msg_text = msg_text.replace(f"<@{bot_uid}>", "").strip() + + blocks = msg.get("blocks") + extras: list[str] = [] + if blocks: + rich_text = _extract_text_from_slack_blocks(blocks).strip() + if rich_text and rich_text not in msg_text: + extras.append(rich_text) + for block in blocks: + block_type = (block or {}).get("type", "") + if block_type in ("section", "header", "context"): + text_obj = block.get("text") or {} + if isinstance(text_obj, dict): + section_text = (text_obj.get("text") or "").strip() + if section_text and section_text not in msg_text and all(section_text not in e for e in extras): + extras.append(section_text) + # Legacy ``attachments`` (Alertmanager, Grafana, PagerDuty, CI bots): + # apps often post with an empty ``text`` and the real content in + # attachment fields or attachment-nested blocks. + attachments_text = _extract_text_from_slack_attachments( + msg.get("attachments") or [] + ).strip() + if attachments_text and attachments_text not in msg_text and all( + attachments_text not in e for e in extras + ): + extras.append(attachments_text) + if blocks: + urls = _extract_urls_from_slack_blocks(blocks) + new_urls = [u for u in urls if u not in msg_text and all(u not in e for e in extras)] + if new_urls: + extras.append("URLs: " + ", ".join(new_urls)) + if extras: + addendum = "\n".join(extras) + msg_text = (msg_text + "\n" + addendum).strip() if msg_text else addendum + + return msg_text + async def _fetch_thread_context( self, channel_id: str, @@ -4502,25 +4594,10 @@ class SlackAdapter(BasePlatformAdapter): ): continue - msg_text = (msg.get("text") or "").strip() - # Apps (Alertmanager, Grafana, CI bots) often post with an empty - # ``text`` and the content in blocks/attachments — fall back so - # messages that started or populate the thread aren't dropped. - if not msg_text: - msg_text = _extract_text_from_slack_blocks( - msg.get("blocks") - ).strip() - if not msg_text: - msg_text = _extract_text_from_slack_attachments( - msg.get("attachments") - ).strip() + msg_text = self._render_message_text(msg, bot_uid=bot_uid) if not msg_text: continue - # Strip bot mentions from context messages - if bot_uid: - msg_text = msg_text.replace(f"<@{bot_uid}>", "").strip() - prefix = "[thread parent] " if is_parent else "" display_user = msg_user or "unknown" # Prefer the bot's own name when the message is a bot post. @@ -4618,17 +4695,7 @@ class SlackAdapter(BasePlatformAdapter): if parent.get("ts", "") != thread_ts: return "" bot_uid = self._team_bot_user_ids.get(team_id, self._bot_user_id) - text = (parent.get("text") or "").strip() - # App-posted parents (e.g. an Alertmanager alert) carry their content - # in blocks/attachments with an empty ``text`` — fall back to those. - if not text: - text = _extract_text_from_slack_blocks(parent.get("blocks")).strip() - if not text: - text = _extract_text_from_slack_attachments( - parent.get("attachments") - ).strip() - if bot_uid: - text = text.replace(f"<@{bot_uid}>", "").strip() + text = self._render_message_text(parent, bot_uid=bot_uid or "") return text except Exception as exc: # pragma: no cover - defensive logger.debug("[Slack] Failed to fetch thread parent text: %s", exc) diff --git a/tests/gateway/test_slack_approval_buttons.py b/tests/gateway/test_slack_approval_buttons.py index 13c62ee1aef07..a1b49806e57dc 100644 --- a/tests/gateway/test_slack_approval_buttons.py +++ b/tests/gateway/test_slack_approval_buttons.py @@ -547,7 +547,145 @@ class TestSlackThreadContext: assert "メール要約: 本日の新着3件" in context @pytest.mark.asyncio - async def test_fetch_thread_context_excludes_self_bot_replies(self): + async def test_fetch_thread_context_extracts_block_kit_parent(self): + """Bot-posted parents that put their content in ``blocks`` (Honeycomb, + PagerDuty, Datadog, GitHub bot, etc.) used to be reduced to just the + ``text`` field — typically only the alert title — which dropped the + URL/button payload that makes the alert useful to an agent replying + in the thread. The fetched context must now include bounded display + text and actionable URLs so section text and button URLs survive.""" + adapter = _make_adapter() + mock_client = adapter._team_clients["T1"] + mock_client.conversations_replies = AsyncMock(return_value={ + "messages": [ + # Bot-posted alert: title in `text`, URL only in `blocks`. + # Mirrors what Honeycomb, PagerDuty, etc. actually send. + { + "ts": "1000.0", + "bot_id": "B_ALERT", + "subtype": "bot_message", + "username": "alertbot", + "text": "low_alerts (checkout)", + "blocks": [ + { + "type": "section", + "text": { + "type": "mrkdwn", + "text": "*Trigger fired:* low_alerts", + }, + }, + { + "type": "actions", + "elements": [ + { + "type": "button", + "text": {"type": "plain_text", "text": "View graph"}, + "url": "https://example.example/view/abc123", + }, + ], + }, + ], + }, + # User reply that triggered the fetch. + {"ts": "1000.1", "user": "U1", "text": "what's going on?"}, + ] + }) + adapter._user_name_cache = {"U1": "Alice"} + + context = await adapter._fetch_thread_context( + channel_id="C1", + thread_ts="1000.0", + current_ts="1000.1", + team_id="T1", + ) + + # Title still present. + assert "low_alerts (checkout)" in context + # URL from the action button must now surface. + assert "https://example.example/view/abc123" in context + # Marked as the thread parent. + assert "[thread parent]" in context + + @pytest.mark.asyncio + async def test_fetch_thread_context_includes_blocks_only_parent(self): + """A parent message with empty ``text`` but non-empty ``blocks`` must + still be included — without this, alerts that put *everything* in + ``blocks`` (some webhook integrations do this) are silently dropped + because the ``if not msg_text: continue`` guard fires.""" + adapter = _make_adapter() + mock_client = adapter._team_clients["T1"] + mock_client.conversations_replies = AsyncMock(return_value={ + "messages": [ + { + "ts": "1000.0", + "bot_id": "B_ALERT", + "subtype": "bot_message", + "username": "alertbot", + "text": "", + "blocks": [ + { + "type": "section", + "text": { + "type": "mrkdwn", + "text": "Build failed: ", + }, + }, + ], + }, + {"ts": "1000.1", "user": "U1", "text": "looking"}, + ] + }) + adapter._user_name_cache = {"U1": "Alice"} + + context = await adapter._fetch_thread_context( + channel_id="C1", + thread_ts="1000.0", + current_ts="1000.1", + team_id="T1", + ) + + assert "[thread parent]" in context + assert "https://example.example/build/9" in context + + @pytest.mark.asyncio + async def test_fetch_thread_parent_text_surfaces_block_urls(self): + """Cold-cache _fetch_thread_parent_text must use the same renderer as + _fetch_thread_context so a bot-posted parent with a URL only in + ``blocks`` surfaces it in reply_to_text, not just in thread context.""" + adapter = _make_adapter() + mock_client = adapter._team_clients["T1"] + mock_client.conversations_replies = AsyncMock(return_value={ + "messages": [ + { + "ts": "1000.0", + "bot_id": "B_ALERT", + "subtype": "bot_message", + "username": "alertbot", + "text": "Incident triggered", + "blocks": [ + { + "type": "actions", + "elements": [ + { + "type": "button", + "text": {"type": "plain_text", "text": "View incident"}, + "url": "https://example.example/incident/42", + }, + ], + }, + ], + }, + ] + }) + + text = await adapter._fetch_thread_parent_text( + channel_id="C1", + thread_ts="1000.0", + team_id="T1", + ) + + assert "Incident triggered" in text + assert "https://example.example/incident/42" in text """Parent (non-self bot) is kept, self-bot child replies are dropped, user replies are kept.""" adapter = _make_adapter()