From 03e5bfff0b5c0ccd7d3bba999726e7b3a4153cec Mon Sep 17 00:00:00 2001 From: gijss <6389756+gijss@users.noreply.github.com> Date: Mon, 25 May 2026 13:15:23 +0200 Subject: [PATCH] fix(gateway): render Slack attachment thread context --- gateway/platforms/slack.py | 276 ++++++++++++------- tests/gateway/test_slack.py | 100 ++++++- tests/gateway/test_slack_approval_buttons.py | 119 ++++++++ 3 files changed, 396 insertions(+), 99 deletions(-) diff --git a/gateway/platforms/slack.py b/gateway/platforms/slack.py index 5accfdb41089..5cbe3e20d0f1 100644 --- a/gateway/platforms/slack.py +++ b/gateway/platforms/slack.py @@ -52,6 +52,13 @@ logger = logging.getLogger(__name__) +_SLACK_RENDER_LOG_PREVIEW_CHARS = 300 +_SLACK_BLOCK_PAYLOAD_MAX_CHARS = 6000 +_SLACK_ATTACHMENT_TEXT_MAX_CHARS = 1500 +_SLACK_ATTACHMENTS_MAX_CHARS = 6000 +_SLACK_TRUNCATED_SUFFIX = "\n... [truncated]" +_SLACK_INLINE_TRUNCATED_SUFFIX = "..." + # ContextVar carrying the user_id of the slash-command invoker. # Set in _handle_slash_command, read in send() to match the correct # stashed response_url when multiple users issue commands on the same @@ -69,7 +76,7 @@ class _ThreadContextCache: content: str fetched_at: float = field(default_factory=time.monotonic) message_count: int = 0 - parent_text: str = "" # Raw text of the thread parent (for reply_to_text injection) + parent_text: str = "" # Rendered thread parent text for reply_to_text injection def check_slack_requirements() -> bool: @@ -140,6 +147,22 @@ def _render_inline_elements(elements: list) -> str: pieces.append(el.get("fallback", "")) return "".join(pieces) + def _collect_block_text(value) -> list[str]: + """Collect readable text from standard Block Kit text objects.""" + if isinstance(value, list): + return [text for item in value for text in _collect_block_text(item)] + if not isinstance(value, dict): + return [] + + if value.get("type") in {"mrkdwn", "plain_text"} and isinstance(value.get("text"), str): + text = value["text"] + return [text] + + collected: list[str] = [] + for item in value.values(): + collected.extend(_collect_block_text(item)) + return collected + def _append_line(text: str, quote_depth: int = 0, bullet: str = "") -> None: if not text or not text.strip(): return @@ -185,11 +208,33 @@ def _walk_elements(elements: list, quote_depth: int = 0, bullet: str = "") -> No for block in blocks: if (block or {}).get("type") == "rich_text": _walk_elements(block.get("elements", [])) + else: + for text in _collect_block_text(block): + _append_line(text) return "\n".join(parts) -def _serialize_slack_blocks_for_agent(blocks: list, max_chars: int = 6000) -> str: +def _truncate_slack_rendered_text(text: str, max_chars: int, suffix: str) -> str: + """Trim rendered Slack context to a bounded prompt-friendly size.""" + text = (text or "").strip() + if len(text) <= max_chars: + return text + cutoff = max(max_chars - len(suffix), 0) + return text[:cutoff].rstrip() + suffix + + +def _slack_log_preview( + text: str, max_chars: int = _SLACK_RENDER_LOG_PREVIEW_CHARS +) -> str: + """Return a single-line preview for debug logs.""" + preview = " ".join((text or "").split()) + return _truncate_slack_rendered_text(preview, max_chars, _SLACK_INLINE_TRUNCATED_SUFFIX) + + +def _serialize_slack_blocks_for_agent( + blocks: list, max_chars: int = _SLACK_BLOCK_PAYLOAD_MAX_CHARS +) -> str: """Return a compact, redacted JSON view of the current message's Block Kit payload.""" if not blocks: return "" @@ -246,12 +291,131 @@ def _sanitize(value): except Exception: payload = repr(blocks) - if len(payload) > max_chars: - payload = payload[: max_chars - 18].rstrip() + "\n... [truncated]" + payload = _truncate_slack_rendered_text(payload, max_chars, _SLACK_TRUNCATED_SUFFIX) return f"[Slack Block Kit payload for this message]\n```json\n{payload}\n```" +def _append_unique_slack_text(base: str, addition: str, separator: str = "\n\n") -> str: + """Append Slack-rendered text when it is not already present.""" + addition = (addition or "").strip() + if not addition: + return (base or "").strip() + base = (base or "").strip() + if addition in base: + return base + if not base: + return addition + return f"{base}{separator}{addition}".strip() + + +def _append_logged_slack_text( + base: str, addition: str, label: str, separator: str = "\n\n" +) -> str: + """Append rendered Slack text and log only when something changed.""" + before = (base or "").strip() + updated = _append_unique_slack_text(before, addition, separator=separator) + if updated != before: + logger.debug("Slack: appended %s: %s", label, _slack_log_preview(addition)) + return updated + + +def _truncate_slack_attachment_text( + text: str, max_chars: int = _SLACK_ATTACHMENT_TEXT_MAX_CHARS +) -> str: + """Bound verbose attachment fields while preserving useful ticket context.""" + return _truncate_slack_rendered_text(text, max_chars, _SLACK_INLINE_TRUNCATED_SUFFIX) + + +def _extract_text_from_slack_attachments( + attachments: list, max_chars: int = _SLACK_ATTACHMENTS_MAX_CHARS +) -> str: + """Render legacy attachment content for agent-visible message text.""" + if not attachments: + return "" + + sections: list[str] = [] + for att in attachments: + if not isinstance(att, dict): + continue + # Skip message-type attachments (e.g. Slack bot messages with + # is_msg_unfurl) to avoid echoing copied Slack messages. + if att.get("is_msg_unfurl"): + continue + + lines: list[str] = [] + title = (att.get("title") or "").strip() + url = (att.get("title_link") or att.get("from_url") or "").strip() + if title and url: + lines.append(f"📎 [{title}]({url})") + elif title: + lines.append(f"📎 {title}") + elif url: + lines.append(f"📎 {url}") + + for key in ("pretext", "text"): + value = _truncate_slack_attachment_text(str(att.get(key) or "")) + if value: + lines.append(value) + + fields = att.get("fields") or [] + if isinstance(fields, list): + for field in fields: + if not isinstance(field, dict): + continue + value = _truncate_slack_attachment_text(str(field.get("value") or "")) + if not value: + continue + field_title = (field.get("title") or "").strip() + lines.append(f"{field_title}: {value}" if field_title else value) + + fallback = _truncate_slack_attachment_text(str(att.get("fallback") or "")) + if fallback and not any(fallback == line or fallback in line for line in lines): + lines.append(fallback) + + footer = (att.get("footer") or "").strip() + if footer: + lines.append(f"_{footer}_") + + if not lines: + continue + + section = "\n ".join(lines) + if not section.startswith("📎 "): + section = f"📎 {section}" + sections.append(section) + + rendered = "\n\n".join(sections).strip() + return _truncate_slack_rendered_text(rendered, max_chars, _SLACK_TRUNCATED_SUFFIX) + + +def _render_slack_message_text( + message: dict, *, include_block_payload: bool = False +) -> str: + """Render user-visible Slack message text from text, blocks, and attachments.""" + text = (message.get("text") or "").strip() + + blocks = message.get("blocks") + if blocks: + blocks_text = _extract_text_from_slack_blocks(blocks) + text = _append_logged_slack_text( + text, blocks_text, "text extracted from blocks", separator="\n" + ) + + if include_block_payload: + blocks_payload = _serialize_slack_blocks_for_agent(blocks) + text = _append_logged_slack_text( + text, blocks_payload, "Block Kit payload to current message text" + ) + + attachments_text = _extract_text_from_slack_attachments( + message.get("attachments") or [] + ) + text = _append_logged_slack_text(text, attachments_text, "attachment text") + + return text.strip() + + 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"): @@ -1818,96 +1982,10 @@ async def _handle_slack_message(self, event: dict) -> None: except Exception: # pragma: no cover - defensive pass - text = original_text - - # Extract quoted/forwarded content from Slack blocks. - # Slack's modern composer embeds forwarded messages in the ``blocks`` - # array as ``rich_text_quote`` elements, which are NOT reflected in - # the plain ``text`` field. Merge block text so the agent sees the - # full message content. - blocks = event.get("blocks") - if blocks: - blocks_text = _extract_text_from_slack_blocks(blocks) - if blocks_text: - # Only append if the blocks contain text not already present - # in the plain text field (avoids duplication). - stripped_blocks = blocks_text.strip() - if stripped_blocks and stripped_blocks not in text.strip(): - logger.debug( - "Slack: extracted additional text from blocks " - "(likely quoted/forwarded content): %s", - stripped_blocks[:300], - ) - text = (text.strip() + "\n" + stripped_blocks).strip() - - blocks_payload = _serialize_slack_blocks_for_agent(blocks) - if blocks_payload: - text = (text.strip() + "\n\n" + blocks_payload).strip() - - # Extract link unfurls / rich attachments (e.g. Notion previews). - # Slack places unfurled link previews in the ``attachments`` array with - # fields like title, title_link/from_url, text, footer, and fallback. - # Without reading these, the agent never sees shared link previews. - slack_attachments = event.get("attachments") or [] - if slack_attachments: - att_parts: list[str] = [] - for att in slack_attachments: - att_title = att.get("title", "") - att_url = att.get("title_link", "") or att.get("from_url", "") - att_text = att.get("text", "") - att_footer = att.get("footer", "") - att_fallback = att.get("fallback", "") - - # Skip message-type attachments (e.g. Slack bot messages with - # is_msg_unfurl) to avoid echoing our own content. - if att.get("is_msg_unfurl"): - continue - - # Build a readable representation. - if att_title and att_url: - header = f"📎 [{att_title}]({att_url})" - elif att_title: - header = f"📎 {att_title}" - elif att_url: - header = f"📎 {att_url}" - else: - header = None - - # Prefer preview text, fall back to fallback description. - body = att_text or att_fallback or "" - if body: - body = body.strip() - if len(body) > 500: - body = body[:497] + "..." - - if header and body: - section = f"{header}\n {body}" - elif header: - section = header - elif body: - section = f"📎 {body}" - else: - continue - - # Deduplicate only when the fully rendered section is already - # present. The shared URL often already appears in the user's - # message text, and skipping on URL/title alone would hide the - # preview body we actually want the agent to see. - if section in text: - continue - - if att_footer: - section = f"{section}\n _{att_footer}_" - - att_parts.append(section) - - if att_parts: - attachment_text = "\n\n".join(att_parts) - text = (text.strip() + "\n\n" + attachment_text).strip() - logger.debug( - "Slack: appended %d link unfurl(s) to message text", - len(att_parts), - ) + text = _render_slack_message_text( + {**event, "text": original_text}, + include_block_payload=True, + ) channel_id = event.get("channel", "") ts = event.get("ts", "") @@ -2675,7 +2753,7 @@ async def _fetch_thread_context( ): continue - msg_text = msg.get("text", "").strip() + msg_text = _render_slack_message_text(msg) if not msg_text: continue @@ -2684,10 +2762,12 @@ async def _fetch_thread_context( msg_text = msg_text.replace(f"<@{bot_uid}>", "").strip() prefix = "[thread parent] " if is_parent else "" - display_user = msg_user or "unknown" + display_user = msg_user # Prefer the bot's own name when the message is a bot post. if is_bot and not display_user: display_user = msg.get("username") or "bot" + if not display_user: + display_user = "unknown" name = await self._resolve_user_name(display_user, chat_id=channel_id) context_parts.append(f"{prefix}{name}: {msg_text}") if is_parent: @@ -2716,7 +2796,7 @@ async def _fetch_thread_context( async def _fetch_thread_parent_text( self, channel_id: str, thread_ts: str, team_id: str = "", ) -> str: - """Return the raw text of the thread parent message (for reply_to_text). + """Return rendered thread parent text for reply_to_text. Uses the same per-thread cache as :meth:`_fetch_thread_context` to avoid hitting ``conversations.replies`` twice. Falls back to a cheap single- @@ -2746,7 +2826,7 @@ async def _fetch_thread_parent_text( 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() + text = _render_slack_message_text(parent) if bot_uid: text = text.replace(f"<@{bot_uid}>", "").strip() return text diff --git a/tests/gateway/test_slack.py b/tests/gateway/test_slack.py index bc09279eec4e..11945e70513b 100644 --- a/tests/gateway/test_slack.py +++ b/tests/gateway/test_slack.py @@ -63,7 +63,10 @@ def _ensure_slack_mock(): import gateway.platforms.slack as _slack_mod _slack_mod.SLACK_AVAILABLE = True -from gateway.platforms.slack import SlackAdapter # noqa: E402 +from gateway.platforms.slack import ( # noqa: E402 + SlackAdapter, + _extract_text_from_slack_attachments, +) # --------------------------------------------------------------------------- @@ -1072,6 +1075,29 @@ async def test_rich_text_quotes_and_lists_are_extracted(self, adapter): assert "• First bullet" in msg_event.text assert "• Second bullet" in msg_event.text + @pytest.mark.asyncio + async def test_current_message_includes_block_kit_payload(self, adapter): + """Current non-rich Block Kit payloads stay visible to the agent.""" + event = self._make_event( + text="Please review this form", + blocks=[ + { + "type": "section", + "text": { + "type": "mrkdwn", + "text": "*Priority:* high", + }, + } + ], + ) + + await adapter._handle_slack_message(event) + + msg_event = adapter.handle_message.call_args[0][0] + assert "Please review this form" in msg_event.text + assert "[Slack Block Kit payload for this message]" in msg_event.text + assert "Priority" in msg_event.text + @pytest.mark.asyncio async def test_attachments_unfurl_text_is_appended_even_when_url_is_in_message(self, adapter): """Shared URLs should still expose unfurl preview text to the agent.""" @@ -1095,6 +1121,78 @@ async def test_attachments_unfurl_text_is_appended_even_when_url_is_in_message(s assert "The latest product spec preview" in msg_event.text assert "_Notion_" in msg_event.text + @pytest.mark.asyncio + async def test_attachment_fields_are_visible_for_integration_messages(self, adapter): + """Integration messages may put the useful payload only in attachment fields.""" + adapter._user_name_cache = {"U_USER": "Requester"} + ticket_link = ( + " " + ) + event = self._make_event( + text="", + attachments=[ + { + "text": "Acme Corp · Jane Requester", + "fields": [ + { + "value": ( + ticket_link + + "The requester reports that adding a foreign " + "account opens an error screen." + ) + } + ], + "footer": ( + " | Status: new" + ), + } + ], + ) + + await adapter._handle_slack_message(event) + + msg_event = adapter.handle_message.call_args[0][0] + assert "Acme Corp · Jane Requester" in msg_event.text + assert "#1234 · Error while adding bank account" in msg_event.text + assert "Ticket #1234" in msg_event.text + + @pytest.mark.asyncio + async def test_attachment_fallback_is_kept_when_fields_are_metadata(self, adapter): + """Fields should not suppress fallback summaries that carry the title.""" + event = self._make_event( + text="", + attachments=[ + { + "fields": [{"title": "Status", "value": "new"}], + "fallback": "Ticket #1234: Error while adding bank account", + } + ], + ) + + await adapter._handle_slack_message(event) + + msg_event = adapter.handle_message.call_args[0][0] + assert "Status: new" in msg_event.text + assert "Ticket #1234: Error while adding bank account" in msg_event.text + + def test_attachment_rendering_truncates_fields_and_total_output(self): + """Long attachment payloads are bounded at field and aggregate levels.""" + long_field_value = "x" * 1600 + rendered = _extract_text_from_slack_attachments([ + {"fields": [{"title": "Details", "value": long_field_value}]} + ]) + assert ("x" * 1497) + "..." in rendered + assert ("x" * 1498) not in rendered + + total_rendered = _extract_text_from_slack_attachments( + [{"text": "y" * 1500} for _ in range(5)], + max_chars=2000, + ) + assert len(total_rendered) <= 2000 + assert total_rendered.endswith("... [truncated]") + @pytest.mark.asyncio async def test_message_unfurl_attachments_are_skipped(self, adapter): """Message unfurls should be skipped to avoid echoing Slack message copies.""" diff --git a/tests/gateway/test_slack_approval_buttons.py b/tests/gateway/test_slack_approval_buttons.py index bc12d0072bd3..fecdc7f80712 100644 --- a/tests/gateway/test_slack_approval_buttons.py +++ b/tests/gateway/test_slack_approval_buttons.py @@ -369,6 +369,96 @@ async def test_fetch_thread_context_includes_bot_parent(self): assert "[thread parent]" in context assert "メール要約: 本日の新着3件" in context + @pytest.mark.asyncio + async def test_fetch_thread_context_renders_attachment_only_parent(self): + """Zendesk-style Slack app parents can have empty text and rich attachments.""" + adapter = _make_adapter() + mock_client = adapter._team_clients["T1"] + ticket_link = ( + " " + ) + mock_client.conversations_replies = AsyncMock(return_value={ + "messages": [ + { + "ts": "1000.0", + "bot_id": "B_ZENDESK", + "subtype": "bot_message", + "username": "Zendesk", + "text": "", + "attachments": [ + { + "text": "Acme Corp · Jane Requester", + "fields": [ + { + "value": ( + ticket_link + + "The requester reports that adding " + "a foreign account opens an error screen." + ) + } + ], + "footer": ( + " | Status: new" + ), + } + ], + }, + {"ts": "1000.1", "user": "U1", "text": "Current"}, + ] + }) + adapter._user_name_cache = {"Zendesk": "Zendesk", "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] Zendesk:" in context + assert "Acme Corp · Jane Requester" in context + assert "#1234 · Error while adding bank account" in context + assert "Ticket #1234" in context + + @pytest.mark.asyncio + async def test_fetch_thread_context_omits_block_payload_json_from_history(self): + """Thread backfill should stay readable and avoid per-message JSON bloat.""" + adapter = _make_adapter() + mock_client = adapter._team_clients["T1"] + mock_client.conversations_replies = AsyncMock(return_value={ + "messages": [ + { + "ts": "1000.0", + "user": "U1", + "text": "Parent with workflow block", + "blocks": [ + { + "type": "section", + "text": { + "type": "mrkdwn", + "text": "*Workflow:* deploy approval", + }, + } + ], + }, + {"ts": "1000.1", "user": "U1", "text": "Current"}, + ] + }) + 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 "Parent with workflow block" in context + assert "*Workflow:* deploy approval" in context + assert "Slack Block Kit payload" not in context + @pytest.mark.asyncio async def test_fetch_thread_context_excludes_self_bot_replies(self): """Parent (non-self bot) is kept, self-bot child replies are dropped, @@ -497,6 +587,35 @@ async def test_fetch_thread_parent_text_from_cache(self): # No additional API call assert mock_client.conversations_replies.await_count == 1 + @pytest.mark.asyncio + async def test_fetch_thread_parent_text_renders_attachment_only_parent(self): + """Cold parent-text fetches should use the same renderer as context fetches.""" + adapter = _make_adapter() + mock_client = adapter._team_clients["T1"] + mock_client.conversations_replies = AsyncMock(return_value={ + "messages": [ + { + "ts": "1000.0", + "bot_id": "B_ZENDESK", + "subtype": "bot_message", + "text": "", + "attachments": [ + { + "text": "Acme Corp · Jane Requester", + "fields": [{"value": "Ticket #1234 details"}], + } + ], + } + ] + }) + + parent = await adapter._fetch_thread_parent_text( + channel_id="C1", thread_ts="1000.0", team_id="T1" + ) + + assert "Acme Corp · Jane Requester" in parent + assert "Ticket #1234 details" in parent + # =========================================================================== # _has_active_session_for_thread — session key fix (#5833)