fix(slack): surface Block Kit content in fetched thread context
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.
This commit is contained in:
Benjamin Ross
Teknium
parent
1f92842c1c
commit
3865694cf9
|
|
@ -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)
|
||||
|
|
|
|||
|
|
@ -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: <https://example.example/build/9|#9>",
|
||||
},
|
||||
},
|
||||
],
|
||||
},
|
||||
{"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()
|
||||
|
|
|
|||
Loading…
Reference in New Issue