From 638af123ba8afd77dbabf8fe13ec3e09bfac6438 Mon Sep 17 00:00:00 2001 From: Max Hsu Date: Wed, 24 Jun 2026 23:53:14 +0800 Subject: [PATCH] fix(dingtalk): preserve richText formatting and set HTTP client timeout richText messages only kept rich_text_list items whose type was "text", so bold/italic/inlineCode/pre segments were silently dropped; a message made entirely of formatted segments produced empty content and fell through to the "unsupported message type: richText" warning. Now any item carrying text is kept (matching the SDK's own get_text_list, which keys off the "text" field) and its type is mapped to Markdown so the formatting survives into the agent's context. Text and downloadCode within a rich-text item are handled independently (the SDK treats them as separate via get_text_list / get_image_list), so an item carrying both a caption and an attachment no longer drops the file. The shared httpx.AsyncClient was created without a timeout, so all requests (including large file/image downloads) used httpx's 5s default and hit ConnectTimeout/ReadTimeout on uploads. Set an explicit httpx.Timeout (connect=10s, read/write=30s). Adds regression tests for formatted-segment preservation, the all-formatted no-drop case, the text+downloadCode item, and the client timeout configuration. Closes #4497 --- nanobot/channels/dingtalk.py | 26 ++++- tests/channels/test_dingtalk_channel.py | 144 ++++++++++++++++++++++++ 2 files changed, 164 insertions(+), 6 deletions(-) diff --git a/nanobot/channels/dingtalk.py b/nanobot/channels/dingtalk.py index 4be3ccb9..401fb375 100644 --- a/nanobot/channels/dingtalk.py +++ b/nanobot/channels/dingtalk.py @@ -94,11 +94,23 @@ class NanobotDingTalkHandler(CallbackHandler): for item in rich_list: if not isinstance(item, dict): continue - if item.get("type") == "text": - t = item.get("text", "").strip() - if t: - content = (content + " " + t).strip() if content else t - elif item.get("downloadCode"): + # A rich-text item may carry text and/or a downloadCode; the + # DingTalk SDK treats them independently, so handle both. + t = item.get("text", "").strip() + if t: + fmt = item.get("type", "") + if fmt == "bold": + formatted = f"**{t}**" + elif fmt == "italic": + formatted = f"*{t}*" + elif fmt == "inlineCode": + formatted = f"`{t}`" + elif fmt == "pre": + formatted = f"```\n{t}\n```" + else: + formatted = t + content = (content + " " + formatted).strip() if content else formatted + if item.get("downloadCode"): dc = item["downloadCode"] fname = item.get("fileName") or "file" sender_uid = chatbot_msg.sender_staff_id or chatbot_msg.sender_id or "unknown" @@ -214,7 +226,9 @@ class DingTalkChannel(BaseChannel): return self._running = True - self._http = httpx.AsyncClient() + self._http = httpx.AsyncClient( + timeout=httpx.Timeout(10.0, connect=10.0, read=30.0, write=30.0, pool=10.0) + ) self.logger.info( "Initializing Stream Client with Client ID: {}...", diff --git a/tests/channels/test_dingtalk_channel.py b/tests/channels/test_dingtalk_channel.py index d3675943..3a464e85 100644 --- a/tests/channels/test_dingtalk_channel.py +++ b/tests/channels/test_dingtalk_channel.py @@ -259,6 +259,150 @@ async def test_handler_processes_file_message(monkeypatch) -> None: assert "/tmp/nanobot_dingtalk/user1/report.xlsx" in msg.content +def _rich_text_message(rich_text_list): + class _FakeRichTextChatbotMessage: + text = None + extensions = {} + image_content = None + rich_text_content = SimpleNamespace(rich_text_list=rich_text_list) + sender_staff_id = "user1" + sender_id = "fallback-user" + sender_nick = "Alice" + message_type = "richText" + + @staticmethod + def from_dict(_data): + return _FakeRichTextChatbotMessage() + + return _FakeRichTextChatbotMessage + + +@pytest.mark.asyncio +async def test_handler_richtext_keeps_formatted_segments(monkeypatch) -> None: + """richText segments with non-'text' types (bold/italic/code/pre) must be kept + and mapped to Markdown, not dropped (issue #4497).""" + bus = MessageBus() + channel = DingTalkChannel( + DingTalkConfig(client_id="app", client_secret="secret", allow_from=["user1"]), + bus, + ) + handler = NanobotDingTalkHandler(channel) + + fake_msg = _rich_text_message([ + {"type": "bold", "text": "Title"}, + {"type": "text", "text": "plain"}, + {"type": "italic", "text": "em"}, + {"type": "inlineCode", "text": "x = 1"}, + {"type": "pre", "text": "block"}, + ]) + monkeypatch.setattr(dingtalk_module, "ChatbotMessage", fake_msg) + monkeypatch.setattr(dingtalk_module, "AckMessage", SimpleNamespace(STATUS_OK="OK")) + + status, body = await handler.process( + SimpleNamespace(data={"conversationType": "1", "text": {"content": ""}}) + ) + msg = await asyncio.wait_for(bus.consume_inbound(), timeout=2.0) + + assert (status, body) == ("OK", "OK") + assert msg.content == "**Title** plain *em* `x = 1` ```\nblock\n```" + + +@pytest.mark.asyncio +async def test_handler_richtext_all_formatted_not_dropped(monkeypatch) -> None: + """A richText message made only of formatted segments must not end up with empty + content and fall through to the 'unsupported message type' path (issue #4497).""" + bus = MessageBus() + channel = DingTalkChannel( + DingTalkConfig(client_id="app", client_secret="secret", allow_from=["user1"]), + bus, + ) + handler = NanobotDingTalkHandler(channel) + + fake_msg = _rich_text_message([{"type": "bold", "text": "Important"}]) + monkeypatch.setattr(dingtalk_module, "ChatbotMessage", fake_msg) + monkeypatch.setattr(dingtalk_module, "AckMessage", SimpleNamespace(STATUS_OK="OK")) + + status, body = await handler.process( + SimpleNamespace(data={"conversationType": "1", "text": {"content": ""}}) + ) + # Before the fix this message produced empty content and never reached the bus, + # so consume_inbound would block here. + msg = await asyncio.wait_for(bus.consume_inbound(), timeout=2.0) + + assert (status, body) == ("OK", "OK") + assert msg.content == "**Important**" + + +@pytest.mark.asyncio +async def test_handler_richtext_item_with_text_and_download(monkeypatch) -> None: + """A rich-text item carrying both text and a downloadCode must yield both the + text and the downloaded file, not drop the attachment (issue #4497).""" + bus = MessageBus() + channel = DingTalkChannel( + DingTalkConfig(client_id="app", client_secret="secret", allow_from=["user1"]), + bus, + ) + handler = NanobotDingTalkHandler(channel) + + fake_msg = _rich_text_message([ + {"text": "see attached", "downloadCode": "abc123", "fileName": "report.xlsx"}, + ]) + + async def fake_download(download_code, filename, sender_id): + return f"/tmp/nanobot_dingtalk/{sender_id}/{filename}" + + monkeypatch.setattr(dingtalk_module, "ChatbotMessage", fake_msg) + monkeypatch.setattr(dingtalk_module, "AckMessage", SimpleNamespace(STATUS_OK="OK")) + monkeypatch.setattr(channel, "_download_dingtalk_file", fake_download) + + status, body = await handler.process( + SimpleNamespace(data={"conversationType": "1", "text": {"content": ""}}) + ) + await asyncio.gather(*list(channel._background_tasks)) + msg = await asyncio.wait_for(bus.consume_inbound(), timeout=2.0) + + assert (status, body) == ("OK", "OK") + assert "see attached" in msg.content + assert "/tmp/nanobot_dingtalk/user1/report.xlsx" in msg.content + + +@pytest.mark.asyncio +async def test_start_configures_http_timeout(monkeypatch) -> None: + """The shared httpx client must be created with an explicit timeout so file/image + downloads don't hit httpx's 5s default and ConnectTimeout (issue #4497).""" + channel = DingTalkChannel( + DingTalkConfig(client_id="app", client_secret="secret", allow_from=["*"]), + MessageBus(), + ) + + class _FakeStreamClient: + def __init__(self, _credential): + pass + + def register_callback_handler(self, _topic, _handler): + pass + + async def start(self): + # Exit the reconnect loop after one iteration. + channel._running = False + + monkeypatch.setattr(dingtalk_module, "DINGTALK_AVAILABLE", True) + monkeypatch.setattr(dingtalk_module, "Credential", lambda *a, **k: object()) + monkeypatch.setattr(dingtalk_module, "DingTalkStreamClient", _FakeStreamClient) + monkeypatch.setattr(dingtalk_module, "ChatbotMessage", SimpleNamespace(TOPIC="topic")) + + await channel.start() + + assert channel._http is not None + timeout = channel._http.timeout + assert timeout.connect == 10.0 + assert timeout.read == 30.0 + assert timeout.write == 30.0 + assert timeout.pool == 10.0 + + await channel.stop() + + @pytest.mark.asyncio async def test_download_dingtalk_file(tmp_path, monkeypatch) -> None: """Test the two-step file download flow (get URL then download content)."""