fix(slack): read thread context from message attachments and blocks - #61261
fix(slack): read thread context from message attachments and blocks#61261xcompass wants to merge 1 commit into
Conversation
`_fetch_thread_context` and `_fetch_thread_parent_text` only read each message's plain `text` field, so messages posted by apps (Alertmanager, Grafana, PagerDuty, CI bots) — which carry their content in legacy `attachments` or Block Kit `blocks` with an empty `text` — were dropped entirely. When such a message *starts* a thread (e.g. an alert), a bot mentioned mid-thread to investigate sees an empty thread and can only ask "what should I investigate?". Fall back to the existing `_extract_text_from_slack_blocks` and a new `_extract_text_from_slack_attachments` helper when `text` is empty, so app-posted alerts and notifications are visible in fetched thread history. Adds TestThreadContextAppMessages (attachment-only, blocks-only, and empty-message cases).
Competing/related cluster for Slack bot-alert thread context: #33493 targets the now-relocated |
teknium1
left a comment
There was a problem hiding this comment.
Thanks for covering the fetched-thread path and the related parent-text path.
Problems
- The Block Kit portion is incomplete:
_extract_text_from_slack_blocksonly processesrich_textblocks atplugins/platforms/slack/adapter.py:203-205, while the new fallback atplugins/platforms/slack/adapter.py:3833-3836relies on it for every block payload. An empty-text standardsectionblock is therefore still dropped. The added blocks test uses onlyrich_textattests/gateway/test_slack.py:4255-4267.
Suggested changes
- Render standard text-bearing Block Kit block types in this fetched-context fallback (or use an existing suitable sanitized representation) and add a
section-block regression test. - Add a cold-cache
_fetch_thread_parent_textattachment/block regression test; the current tests cover_fetch_thread_contextonly.
Automated hermes-sweeper review.
| # ``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( |
There was a problem hiding this comment.
_extract_text_from_slack_blocks currently renders only rich_text blocks (adapter.py:203-205). This leaves ordinary text-bearing section blocks empty here, so the message is still skipped. Please cover standard Block Kit text blocks as well and add a matching regression test.
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.
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.
|
Merged via #69316 — your commit was cherry-picked onto current main with your authorship preserved in git history: your attachments+blocks thread-context fallback was cherry-picked directly. Thanks for the contribution! |
|
Thanks! |
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 NousResearch#61261 during conflict resolution) legacy
attachment fields, and use it for thread-context and parent-text
rendering.
Salvaged from NousResearch#29541.
Problem
When the bot is @mentioned mid-thread for the first time,
_fetch_thread_contextfetches prior thread messages (
conversations.replies) and prepends them as context —but it reads only each message's plain
textfield:Apps like Alertmanager, Grafana, PagerDuty, and CI bots post with an empty
textand the real content in legacy
attachmentsor Block Kitblocks. Those messagesare dropped — so when such a message starts the thread (e.g. a firing alert), an agent
asked to investigate sees an empty thread and can only reply "What should I investigate?".
Observed on a real Alertmanager thread (details genericized):
text""attachments[0]→[FIRING:1] KubeJobFailed cluster-01 batch-job-123456@bot investigate_fetch_thread_parent_texthas the same gap.Fix
When
textis empty, fall back to:_extract_text_from_slack_blocks(msg["blocks"])— already used for live incoming messages;_extract_text_from_slack_attachments(msg["attachments"])helper that pullspretext/title/text/fields(plus nested blocks), using thefallbackstring onlywhen an attachment has nothing structured.
Applied in both
_fetch_thread_contextand_fetch_thread_parent_text. No behavior changefor messages that already have
text.Tests
New
TestThreadContextAppMessagescovers attachment-only (Alertmanager-style), blocks-only,and truly-empty messages.
tests/gateway/test_slack.pypasses (incl. existing thread-contexttests) and
ruff checkis clean.