mirror of
https://github.com/bytedance/deer-flow.git
synced 2026-07-25 23:48:00 +00:00
fix(feishu): check response.success() on card/reaction SDK calls (#4234)
* fix(feishu): check response.success() on card/reaction SDK calls _reply_card, _create_card, _update_card, and _add_reaction call the lark-oapi SDK and only used the response on the happy path, never checking response.success(). lark-oapi signals a business-level failure (invalid/expired card, permission error, etc.) by returning a response with success()=False rather than raising, so these calls looked identical to callers whether Feishu accepted them or not. This file's own _upload_image/_upload_file/_receive_single_file already guard against exactly this by checking response.success() before trusting the response; the card/reaction helpers just didn't follow that established pattern. The gap is most exposed on _update_card: Feishu supports streaming, so a single conversation issues many _update_card patches, each one a chance to silently drop an update. _send_card_message already has a try/except around _update_card that retries (via _send_with_retry) on non-final failures and falls back to a brand-new card on final ones - but that logic was unreachable because _update_card could never raise on a business failure. Adds response.success() checks to all four methods, raising for _reply_card/_create_card/_update_card (mirroring the upload helpers, and making the existing retry/fallback logic in _send_card_message reachable) and logging a warning for _add_reaction (mirroring _receive_single_file, since a failed reaction is fire-and-forget and must not trigger a redundant resend of the whole card). Adds regression coverage in TestFeishuCardSuccessChecks: a business-failure mock response for each of the four methods, plus two tests driving _send_card_message end to end to confirm the retry and fallback-to-new-card paths actually engage now. * fix(feishu): include log_id in card SDK failure errors + cover create_card retry path willem-bd's review on this PR suggested two non-blocking follow-ups: - _reply_card/_create_card/_update_card's RuntimeError on a business-level failure omitted the Feishu log_id, unlike _add_reaction and _receive_single_file in this same file, which already include it in their warning logs. Adding it gives a Feishu support-traceable id once retries exhaust and the error reaches the caller. - _create_card's failure on the no-thread_ts path (the tail of _send_card_message) only had direct unit coverage (test_create_card_raises_on_business_failure_response), unlike _update_card's failure path, which also has an end-to-end test through send() confirming _send_with_retry engages (test_send_retries_after_update_card_business_failure_then_succeeds). Adds the mirrored end-to-end test for the _create_card path.
This commit is contained in:
Daoyuan Li
GitHub
parent
09d9cf53d2
commit
314f84bc8d
@ -493,7 +493,17 @@ class FeishuChannel(Channel):
|
||||
return
|
||||
try:
|
||||
request = self._CreateMessageReactionRequest.builder().message_id(message_id).request_body(self._CreateMessageReactionRequestBody.builder().reaction_type(self._Emoji.builder().emoji_type(emoji_type).build()).build()).build()
|
||||
await asyncio.to_thread(self._api_client.im.v1.message_reaction.create, request)
|
||||
response = await asyncio.to_thread(self._api_client.im.v1.message_reaction.create, request)
|
||||
if not response.success():
|
||||
logger.warning(
|
||||
"[Feishu] reaction '%s' add failed for message %s: code=%s, msg=%s, log_id=%s",
|
||||
emoji_type,
|
||||
message_id,
|
||||
response.code,
|
||||
response.msg,
|
||||
response.get_log_id(),
|
||||
)
|
||||
return
|
||||
logger.info("[Feishu] reaction '%s' added to message %s", emoji_type, message_id)
|
||||
except Exception:
|
||||
logger.exception("[Feishu] failed to add reaction '%s' to message %s", emoji_type, message_id)
|
||||
@ -506,6 +516,8 @@ class FeishuChannel(Channel):
|
||||
content = self._build_card_content(text)
|
||||
request = self._ReplyMessageRequest.builder().message_id(message_id).request_body(self._ReplyMessageRequestBody.builder().msg_type("interactive").content(content).build()).build()
|
||||
response = await asyncio.to_thread(self._api_client.im.v1.message.reply, request)
|
||||
if not response.success():
|
||||
raise RuntimeError(f"Feishu card reply failed: code={response.code}, msg={response.msg}, log_id={response.get_log_id()}")
|
||||
response_data = getattr(response, "data", None)
|
||||
return getattr(response_data, "message_id", None)
|
||||
|
||||
@ -516,7 +528,9 @@ class FeishuChannel(Channel):
|
||||
|
||||
content = self._build_card_content(text)
|
||||
request = self._CreateMessageRequest.builder().receive_id_type("chat_id").request_body(self._CreateMessageRequestBody.builder().receive_id(chat_id).msg_type("interactive").content(content).build()).build()
|
||||
await asyncio.to_thread(self._api_client.im.v1.message.create, request)
|
||||
response = await asyncio.to_thread(self._api_client.im.v1.message.create, request)
|
||||
if not response.success():
|
||||
raise RuntimeError(f"Feishu card creation failed: code={response.code}, msg={response.msg}, log_id={response.get_log_id()}")
|
||||
|
||||
async def _update_card(self, message_id: str, text: str) -> None:
|
||||
"""Patch an existing card message in place."""
|
||||
@ -525,7 +539,9 @@ class FeishuChannel(Channel):
|
||||
|
||||
content = self._build_card_content(text)
|
||||
request = self._PatchMessageRequest.builder().message_id(message_id).request_body(self._PatchMessageRequestBody.builder().content(content).build()).build()
|
||||
await asyncio.to_thread(self._api_client.im.v1.message.patch, request)
|
||||
response = await asyncio.to_thread(self._api_client.im.v1.message.patch, request)
|
||||
if not response.success():
|
||||
raise RuntimeError(f"Feishu card update failed: code={response.code}, msg={response.msg}, log_id={response.get_log_id()}")
|
||||
|
||||
def _track_background_task(self, task: asyncio.Task, *, name: str, msg_id: str) -> None:
|
||||
"""Keep a strong reference to fire-and-forget tasks and surface errors."""
|
||||
|
||||
@ -5457,6 +5457,267 @@ class TestFeishuSendFileSuccessChecks:
|
||||
_run(go())
|
||||
|
||||
|
||||
class TestFeishuCardSuccessChecks:
|
||||
"""Regression coverage: ``lark-oapi`` signals a *business-level* failure
|
||||
(expired/invalid card, permission error, etc.) by returning a response
|
||||
whose ``response.success()`` is ``False`` -- the SDK call itself does not
|
||||
raise. This file's own ``_upload_image``/``_upload_file``/
|
||||
``_receive_single_file`` already guard against this by checking
|
||||
``response.success()``; ``_reply_card``/``_create_card``/``_update_card``/
|
||||
``_add_reaction`` did not, so a failed card send/update looked identical
|
||||
to a successful one to every caller.
|
||||
"""
|
||||
|
||||
def test_reply_card_raises_on_business_failure_response(self):
|
||||
from lark_oapi.api.im.v1 import ReplyMessageRequest, ReplyMessageRequestBody
|
||||
|
||||
from app.channels.feishu import FeishuChannel
|
||||
|
||||
async def go():
|
||||
bus = MessageBus()
|
||||
channel = FeishuChannel(bus, config={})
|
||||
channel._api_client = MagicMock()
|
||||
channel._ReplyMessageRequest = ReplyMessageRequest
|
||||
channel._ReplyMessageRequestBody = ReplyMessageRequestBody
|
||||
|
||||
failure_response = MagicMock()
|
||||
failure_response.success.return_value = False
|
||||
failure_response.code = 99991400
|
||||
failure_response.msg = "param invalid"
|
||||
failure_response.get_log_id.return_value = "log-reply-1"
|
||||
channel._api_client.im.v1.message.reply = MagicMock(return_value=failure_response)
|
||||
|
||||
with pytest.raises(RuntimeError, match="99991400") as exc_info:
|
||||
await channel._reply_card("om-source-msg", "hello")
|
||||
assert "log-reply-1" in str(exc_info.value)
|
||||
|
||||
_run(go())
|
||||
|
||||
def test_create_card_raises_on_business_failure_response(self):
|
||||
from lark_oapi.api.im.v1 import CreateMessageRequest, CreateMessageRequestBody
|
||||
|
||||
from app.channels.feishu import FeishuChannel
|
||||
|
||||
async def go():
|
||||
bus = MessageBus()
|
||||
channel = FeishuChannel(bus, config={})
|
||||
channel._api_client = MagicMock()
|
||||
channel._CreateMessageRequest = CreateMessageRequest
|
||||
channel._CreateMessageRequestBody = CreateMessageRequestBody
|
||||
|
||||
failure_response = MagicMock()
|
||||
failure_response.success.return_value = False
|
||||
failure_response.code = 99991400
|
||||
failure_response.msg = "param invalid"
|
||||
failure_response.get_log_id.return_value = "log-create-1"
|
||||
channel._api_client.im.v1.message.create = MagicMock(return_value=failure_response)
|
||||
|
||||
with pytest.raises(RuntimeError, match="99991400") as exc_info:
|
||||
await channel._create_card("chat-1", "hello")
|
||||
assert "log-create-1" in str(exc_info.value)
|
||||
|
||||
_run(go())
|
||||
|
||||
def test_update_card_raises_on_business_failure_response(self):
|
||||
from lark_oapi.api.im.v1 import PatchMessageRequest, PatchMessageRequestBody
|
||||
|
||||
from app.channels.feishu import FeishuChannel
|
||||
|
||||
async def go():
|
||||
bus = MessageBus()
|
||||
channel = FeishuChannel(bus, config={})
|
||||
channel._api_client = MagicMock()
|
||||
channel._PatchMessageRequest = PatchMessageRequest
|
||||
channel._PatchMessageRequestBody = PatchMessageRequestBody
|
||||
|
||||
failure_response = MagicMock()
|
||||
failure_response.success.return_value = False
|
||||
failure_response.code = 99991400
|
||||
failure_response.msg = "card has expired"
|
||||
failure_response.get_log_id.return_value = "log-update-1"
|
||||
channel._api_client.im.v1.message.patch = MagicMock(return_value=failure_response)
|
||||
|
||||
with pytest.raises(RuntimeError, match="99991400") as exc_info:
|
||||
await channel._update_card("om-running-card", "hello")
|
||||
assert "log-update-1" in str(exc_info.value)
|
||||
|
||||
_run(go())
|
||||
|
||||
def test_add_reaction_logs_warning_on_business_failure_without_raising(self, caplog):
|
||||
from lark_oapi.api.im.v1 import (
|
||||
CreateMessageReactionRequest,
|
||||
CreateMessageReactionRequestBody,
|
||||
Emoji,
|
||||
)
|
||||
|
||||
from app.channels.feishu import FeishuChannel
|
||||
|
||||
async def go():
|
||||
bus = MessageBus()
|
||||
channel = FeishuChannel(bus, config={})
|
||||
channel._api_client = MagicMock()
|
||||
channel._CreateMessageReactionRequest = CreateMessageReactionRequest
|
||||
channel._CreateMessageReactionRequestBody = CreateMessageReactionRequestBody
|
||||
channel._Emoji = Emoji
|
||||
|
||||
failure_response = MagicMock()
|
||||
failure_response.success.return_value = False
|
||||
failure_response.code = 99991400
|
||||
failure_response.msg = "reaction not allowed"
|
||||
failure_response.get_log_id.return_value = "log-1"
|
||||
channel._api_client.im.v1.message_reaction.create = MagicMock(return_value=failure_response)
|
||||
|
||||
with caplog.at_level(logging.WARNING):
|
||||
await channel._add_reaction("om-source-msg", "OK")
|
||||
|
||||
assert "99991400" in caplog.text
|
||||
|
||||
_run(go())
|
||||
|
||||
def test_final_streaming_update_falls_back_to_new_card_when_update_card_fails(self):
|
||||
"""``_send_card_message``'s ``try/except`` around ``_update_card``
|
||||
already falls back to a brand-new card reply for a final message --
|
||||
but that fallback could never fire while ``_update_card`` swallowed
|
||||
business failures silently. Now that ``_update_card`` raises, the
|
||||
fallback is reachable."""
|
||||
from app.channels.feishu import FeishuChannel
|
||||
|
||||
async def go():
|
||||
bus = MessageBus()
|
||||
channel = FeishuChannel(bus, config={})
|
||||
channel._api_client = MagicMock()
|
||||
|
||||
channel._running_card_ids["om-source-msg"] = "om-running-card"
|
||||
channel._update_card = AsyncMock(side_effect=RuntimeError("Feishu card update failed: code=99991400, msg=card expired"))
|
||||
channel._reply_card = AsyncMock(return_value="om-fallback-card")
|
||||
channel._add_reaction = AsyncMock()
|
||||
|
||||
msg = OutboundMessage(
|
||||
channel_name="feishu",
|
||||
chat_id="chat-1",
|
||||
thread_id="thread-1",
|
||||
text="final answer",
|
||||
is_final=True,
|
||||
thread_ts="om-source-msg",
|
||||
)
|
||||
|
||||
await channel._send_card_message(msg)
|
||||
|
||||
channel._update_card.assert_awaited_once_with("om-running-card", "final answer")
|
||||
channel._reply_card.assert_awaited_once_with("om-source-msg", "final answer")
|
||||
assert "om-source-msg" not in channel._running_card_ids
|
||||
|
||||
_run(go())
|
||||
|
||||
def test_non_final_streaming_update_failure_propagates_instead_of_silently_succeeding(self):
|
||||
"""A non-final ``_update_card`` failure must propagate out of
|
||||
``_send_card_message`` so ``send()``'s ``_send_with_retry`` sees it --
|
||||
previously it never would, since ``_update_card`` had no way to raise
|
||||
on a business-level failure."""
|
||||
from app.channels.feishu import FeishuChannel
|
||||
|
||||
async def go():
|
||||
bus = MessageBus()
|
||||
channel = FeishuChannel(bus, config={})
|
||||
channel._api_client = MagicMock()
|
||||
|
||||
channel._running_card_ids["om-source-msg"] = "om-running-card"
|
||||
channel._update_card = AsyncMock(side_effect=RuntimeError("Feishu card update failed: code=99991400, msg=card expired"))
|
||||
channel._reply_card = AsyncMock()
|
||||
|
||||
msg = OutboundMessage(
|
||||
channel_name="feishu",
|
||||
chat_id="chat-1",
|
||||
thread_id="thread-1",
|
||||
text="partial answer",
|
||||
is_final=False,
|
||||
thread_ts="om-source-msg",
|
||||
)
|
||||
|
||||
with pytest.raises(RuntimeError, match="99991400"):
|
||||
await channel._send_card_message(msg)
|
||||
|
||||
channel._reply_card.assert_not_awaited()
|
||||
assert channel._running_card_ids["om-source-msg"] == "om-running-card"
|
||||
|
||||
_run(go())
|
||||
|
||||
def test_send_retries_after_update_card_business_failure_then_succeeds(self, monkeypatch):
|
||||
"""End-to-end through ``send()``: a non-final ``_update_card``
|
||||
business failure must now engage ``_send_with_retry`` instead of the
|
||||
caller believing the streaming update was delivered."""
|
||||
from app.channels.feishu import FeishuChannel
|
||||
|
||||
async def go():
|
||||
bus = MessageBus()
|
||||
channel = FeishuChannel(bus, config={})
|
||||
channel._api_client = MagicMock()
|
||||
sleep = AsyncMock()
|
||||
monkeypatch.setattr("app.channels.base.asyncio.sleep", sleep)
|
||||
|
||||
channel._running_card_ids["om-source-msg"] = "om-running-card"
|
||||
channel._update_card = AsyncMock(
|
||||
side_effect=[
|
||||
RuntimeError("Feishu card update failed: code=99991400, msg=card expired"),
|
||||
None,
|
||||
]
|
||||
)
|
||||
|
||||
msg = OutboundMessage(
|
||||
channel_name="feishu",
|
||||
chat_id="chat-1",
|
||||
thread_id="thread-1",
|
||||
text="partial answer",
|
||||
is_final=False,
|
||||
thread_ts="om-source-msg",
|
||||
)
|
||||
|
||||
await channel.send(msg, _max_retries=2)
|
||||
|
||||
assert channel._update_card.await_count == 2
|
||||
sleep.assert_awaited_once_with(1)
|
||||
|
||||
_run(go())
|
||||
|
||||
def test_send_retries_after_create_card_business_failure_then_succeeds(self, monkeypatch):
|
||||
"""End-to-end through ``send()`` for the no-``thread_ts`` path: a
|
||||
business failure from ``_create_card`` (unwrapped at the tail of
|
||||
``_send_card_message``) must also engage ``_send_with_retry``,
|
||||
mirroring ``test_send_retries_after_update_card_business_failure_then_succeeds``
|
||||
for the ``_update_card`` path above."""
|
||||
from app.channels.feishu import FeishuChannel
|
||||
|
||||
async def go():
|
||||
bus = MessageBus()
|
||||
channel = FeishuChannel(bus, config={})
|
||||
channel._api_client = MagicMock()
|
||||
sleep = AsyncMock()
|
||||
monkeypatch.setattr("app.channels.base.asyncio.sleep", sleep)
|
||||
|
||||
channel._create_card = AsyncMock(
|
||||
side_effect=[
|
||||
RuntimeError("Feishu card creation failed: code=99991400, msg=param invalid, log_id=log-1"),
|
||||
None,
|
||||
]
|
||||
)
|
||||
|
||||
msg = OutboundMessage(
|
||||
channel_name="feishu",
|
||||
chat_id="chat-1",
|
||||
thread_id="thread-1",
|
||||
text="new card message",
|
||||
is_final=True,
|
||||
thread_ts=None,
|
||||
)
|
||||
|
||||
await channel.send(msg, _max_retries=2)
|
||||
|
||||
assert channel._create_card.await_count == 2
|
||||
sleep.assert_awaited_once_with(1)
|
||||
|
||||
_run(go())
|
||||
|
||||
|
||||
class TestWeComChannel:
|
||||
def test_publish_ws_inbound_starts_stream_and_publishes_message(self, monkeypatch):
|
||||
from app.channels.wecom import WeComChannel
|
||||
|
||||
Loading…
x
Reference in New Issue
Block a user