fix(channels): preserve Feishu clarification thread continuity (#3285)

* fix(channels): preserve Feishu clarification thread continuity

* fix(channels): address Feishu clarification review feedback

---------

Co-authored-by: zzp1221 <zzp1221@users.noreply.github.com>
Co-authored-by: Willem Jiang <willem.jiang@gmail.com>
This commit is contained in:
kia
2026-05-31 22:43:07 +08:00
committed by GitHub
parent 79cc227917
commit 46ddc346ad
5 changed files with 685 additions and 16 deletions
+188 -2
View File
@@ -12,7 +12,14 @@ from unittest.mock import AsyncMock, MagicMock, patch
import pytest
from app.channels.base import Channel
from app.channels.message_bus import InboundMessage, InboundMessageType, MessageBus, OutboundMessage, ResolvedAttachment
from app.channels.message_bus import (
PENDING_CLARIFICATION_METADATA_KEY,
InboundMessage,
InboundMessageType,
MessageBus,
OutboundMessage,
ResolvedAttachment,
)
from app.channels.store import ChannelStore
@@ -392,6 +399,47 @@ class TestExtractResponseText:
assert _extract_response_text(result) == "Here is the plan."
class TestClarificationDetection:
def test_final_clarification_tool_message_is_pending(self):
from app.channels.manager import _has_current_turn_clarification
result = {
"messages": [
{"type": "human", "content": "deploy"},
{"type": "ai", "content": "", "tool_calls": [{"name": "ask_clarification", "args": {}}]},
{"type": "tool", "name": "ask_clarification", "content": "Which environment?"},
]
}
assert _has_current_turn_clarification(result) is True
def test_clarification_followed_by_regular_ai_is_not_pending(self):
from app.channels.manager import _has_current_turn_clarification
result = {
"messages": [
{"type": "human", "content": "deploy"},
{"type": "ai", "content": "", "tool_calls": [{"name": "ask_clarification", "args": {}}]},
{"type": "tool", "name": "ask_clarification", "content": "Which environment?"},
{"type": "ai", "content": "I will continue without pending clarification."},
]
}
assert _has_current_turn_clarification(result) is False
def test_previous_turn_clarification_does_not_mark_current_turn(self):
from app.channels.manager import _has_current_turn_clarification
result = {
"messages": [
{"type": "human", "content": "deploy"},
{"type": "ai", "content": "", "tool_calls": [{"name": "ask_clarification", "args": {}}]},
{"type": "tool", "name": "ask_clarification", "content": "Which environment?"},
{"type": "human", "content": "prod"},
{"type": "ai", "content": "Deploying to prod."},
]
}
assert _has_current_turn_clarification(result) is False
# ---------------------------------------------------------------------------
# ChannelManager tests
# ---------------------------------------------------------------------------
@@ -637,6 +685,74 @@ class TestChannelManager:
_run(go())
def test_handle_chat_marks_clarification_outbound_metadata(self):
from app.channels.manager import ChannelManager
async def go():
bus = MessageBus()
store = ChannelStore(path=Path(tempfile.mkdtemp()) / "store.json")
manager = ChannelManager(bus=bus, store=store)
outbound_received: list[OutboundMessage] = []
async def capture_outbound(msg: OutboundMessage) -> None:
outbound_received.append(msg)
bus.subscribe_outbound(capture_outbound)
mock_client = _make_mock_langgraph_client(
run_result={
"messages": [
{"type": "human", "content": "deploy"},
{"type": "ai", "content": "", "tool_calls": [{"name": "ask_clarification", "args": {}}]},
{"type": "tool", "name": "ask_clarification", "content": "Which environment?"},
]
}
)
manager._client = mock_client
await manager.start()
inbound = InboundMessage(
channel_name="test",
chat_id="chat1",
user_id="user1",
text="deploy",
metadata={"message_id": "msg-1"},
)
await bus.publish_inbound(inbound)
await _wait_for(lambda: len(outbound_received) >= 1)
await manager.stop()
assert outbound_received[0].text == "Which environment?"
assert outbound_received[0].metadata["message_id"] == "msg-1"
assert outbound_received[0].metadata[PENDING_CLARIFICATION_METADATA_KEY] is True
_run(go())
def test_handle_chat_does_not_mark_regular_outbound_as_clarification(self):
from app.channels.manager import ChannelManager
async def go():
bus = MessageBus()
store = ChannelStore(path=Path(tempfile.mkdtemp()) / "store.json")
manager = ChannelManager(bus=bus, store=store)
outbound_received: list[OutboundMessage] = []
async def capture_outbound(msg: OutboundMessage) -> None:
outbound_received.append(msg)
bus.subscribe_outbound(capture_outbound)
mock_client = _make_mock_langgraph_client()
manager._client = mock_client
await manager.start()
await bus.publish_inbound(InboundMessage(channel_name="test", chat_id="chat1", user_id="user1", text="hi"))
await _wait_for(lambda: len(outbound_received) >= 1)
await manager.stop()
assert outbound_received[0].text == "Hello from agent!"
assert PENDING_CLARIFICATION_METADATA_KEY not in outbound_received[0].metadata
_run(go())
def test_handle_chat_outbound_drops_large_metadata_keys(self):
"""Large metadata keys like raw_message should be stripped from outbound messages."""
from app.channels.manager import ChannelManager
@@ -1018,6 +1134,67 @@ class TestChannelManager:
_run(go())
def test_handle_feishu_streaming_marks_only_final_clarification_outbound(self, monkeypatch):
from app.channels.manager import ChannelManager
monkeypatch.setattr("app.channels.manager.STREAM_UPDATE_MIN_INTERVAL_SECONDS", 0.0)
async def go():
bus = MessageBus()
store = ChannelStore(path=Path(tempfile.mkdtemp()) / "store.json")
manager = ChannelManager(bus=bus, store=store)
outbound_received: list[OutboundMessage] = []
async def capture_outbound(msg: OutboundMessage) -> None:
outbound_received.append(msg)
bus.subscribe_outbound(capture_outbound)
stream_events = [
_make_stream_part(
"messages-tuple",
[
{"id": "ai-1", "content": "Thinking", "type": "AIMessageChunk"},
{"langgraph_node": "agent"},
],
),
_make_stream_part(
"values",
{
"messages": [
{"type": "human", "content": "deploy"},
{"type": "ai", "content": "", "tool_calls": [{"name": "ask_clarification", "args": {}}]},
{"type": "tool", "name": "ask_clarification", "content": "Which environment?"},
],
"artifacts": [],
},
),
]
mock_client = _make_mock_langgraph_client()
mock_client.runs.stream = MagicMock(return_value=_make_async_iterator(stream_events))
manager._client = mock_client
await manager.start()
await bus.publish_inbound(
InboundMessage(
channel_name="feishu",
chat_id="chat1",
user_id="user1",
text="deploy",
thread_ts="om-source-1",
)
)
await _wait_for(lambda: len(outbound_received) >= 2)
await manager.stop()
assert [msg.is_final for msg in outbound_received] == [False, False, True]
assert outbound_received[0].text == "Thinking"
assert outbound_received[1].text == "Which environment?"
assert outbound_received[2].text == "Which environment?"
assert all(PENDING_CLARIFICATION_METADATA_KEY not in msg.metadata for msg in outbound_received[:-1])
assert outbound_received[-1].metadata[PENDING_CLARIFICATION_METADATA_KEY] is True
_run(go())
def test_handle_feishu_stream_error_still_sends_final(self, monkeypatch):
"""When the stream raises mid-way, a final outbound with is_final=True must still be published."""
from app.channels.manager import ChannelManager
@@ -2010,7 +2187,8 @@ class TestFeishuChannel:
async def go():
bus = MessageBus()
bus.publish_inbound = AsyncMock()
channel = FeishuChannel(bus, config={})
store = ChannelStore(path=Path(tempfile.mkdtemp()) / "store.json")
channel = FeishuChannel(bus, config={"channel_store": store})
channel._api_client = MagicMock()
reply_started = asyncio.Event()
@@ -2046,6 +2224,11 @@ class TestFeishuChannel:
text="Hello",
is_final=False,
thread_ts="om-source-msg",
metadata={
"user_id": "user-1",
"root_id": "om-root-msg",
"topic_id": "om-root-msg",
},
)
)
)
@@ -2060,6 +2243,9 @@ class TestFeishuChannel:
assert channel._reply_card.await_count == 1
channel._update_card.assert_awaited_once_with("om-running-card", "Hello")
assert "om-source-msg" not in channel._running_card_tasks
assert store.get_thread_id("feishu", "chat-1", topic_id="om-source-msg") == "thread-1"
assert store.get_thread_id("feishu", "chat-1", topic_id="om-running-card") == "thread-1"
assert store.get_thread_id("feishu", "chat-1", topic_id="om-root-msg") == "thread-1"
_run(go())
+245 -1
View File
@@ -1,12 +1,38 @@
import asyncio
import json
import tempfile
from pathlib import Path
from unittest.mock import AsyncMock, MagicMock
import pytest
from app.channels.commands import KNOWN_CHANNEL_COMMANDS
from app.channels.feishu import FeishuChannel
from app.channels.message_bus import InboundMessage, MessageBus
from app.channels.message_bus import (
PENDING_CLARIFICATION_METADATA_KEY,
RESOLVED_FROM_PENDING_CLARIFICATION_METADATA_KEY,
InboundMessage,
MessageBus,
OutboundMessage,
)
from app.channels.store import ChannelStore
def _pending(
topic_id: str,
*,
thread_id: str | None = None,
source_message_id: str | None = None,
card_message_id: str | None = None,
created_at: float = 9999999999,
) -> dict:
return {
"thread_id": thread_id or f"deer-thread-{topic_id}",
"topic_id": topic_id,
"source_message_id": source_message_id or topic_id,
"card_message_id": card_message_id or f"card-{topic_id}",
"created_at": created_at,
}
def _run(coro):
@@ -138,6 +164,224 @@ def test_feishu_on_message_extracts_image_and_file_keys():
assert "[file]" in mock_make_inbound.call_args[1]["text"]
def test_feishu_on_message_reuses_stored_parent_topic_for_card_replies():
bus = MessageBus()
store = ChannelStore(path=Path(tempfile.mkdtemp()) / "store.json")
store.set_thread_id(
"feishu",
"chat_1",
"deer-thread-1",
topic_id="om_clarification_card",
user_id="user_1",
)
channel = FeishuChannel(
bus,
{"app_id": "test", "app_secret": "test", "channel_store": store},
)
event = MagicMock()
event.event.message.chat_id = "chat_1"
event.event.message.message_id = "msg_reply"
event.event.message.root_id = "om_unknown_root"
event.event.message.parent_id = "om_clarification_card"
event.event.message.thread_id = None
event.event.sender.sender_id.open_id = "user_1"
event.event.message.content = json.dumps({"text": "prod"})
with pytest.MonkeyPatch.context() as m:
mock_make_inbound = MagicMock()
m.setattr(channel, "_make_inbound", mock_make_inbound)
channel._on_message(event)
inbound = mock_make_inbound.return_value
assert inbound.topic_id == "om_clarification_card"
assert mock_make_inbound.call_args.kwargs["metadata"]["topic_id"] == "om_clarification_card"
def _make_text_event(
text: str,
*,
chat_id: str = "chat_1",
message_id: str = "msg_1",
user_id: str = "user_1",
root_id: str | None = None,
parent_id: str | None = None,
thread_id: str | None = None,
):
event = MagicMock()
event.event.message.chat_id = chat_id
event.event.message.message_id = message_id
event.event.message.root_id = root_id
event.event.message.parent_id = parent_id
event.event.message.thread_id = thread_id
event.event.sender.sender_id.open_id = user_id
event.event.message.content = json.dumps({"text": text})
return event
def test_feishu_plain_reply_consumes_pending_clarification_topic():
bus = MessageBus()
store = ChannelStore(path=Path(tempfile.mkdtemp()) / "store.json")
store.set_thread_id("feishu", "chat_1", "deer-thread-1", topic_id="om_original", user_id="user_1")
channel = FeishuChannel(bus, {"app_id": "test", "app_secret": "test", "channel_store": store})
channel._pending_clarifications[channel._pending_key("chat_1", "user_1")] = [_pending("om_original", thread_id="deer-thread-1", card_message_id="om_card")]
with pytest.MonkeyPatch.context() as m:
mock_make_inbound = MagicMock()
m.setattr(channel, "_make_inbound", mock_make_inbound)
channel._on_message(_make_text_event("2", message_id="msg_plain_2"))
inbound = mock_make_inbound.return_value
metadata = mock_make_inbound.call_args.kwargs["metadata"]
assert inbound.topic_id == "om_original"
assert metadata["topic_id"] == "om_original"
assert metadata[RESOLVED_FROM_PENDING_CLARIFICATION_METADATA_KEY] is True
assert channel._pending_key("chat_1", "user_1") not in channel._pending_clarifications
def test_feishu_pending_clarification_is_consumed_once():
bus = MessageBus()
channel = FeishuChannel(bus, {"app_id": "test", "app_secret": "test"})
channel._pending_clarifications[channel._pending_key("chat_1", "user_1")] = [_pending("om_original", thread_id="deer-thread-1", card_message_id="om_card")]
with pytest.MonkeyPatch.context() as m:
created = []
def fake_make_inbound(**kwargs):
inbound = InboundMessage(channel_name="feishu", **kwargs)
created.append(inbound)
return inbound
mock_make_inbound = MagicMock(side_effect=fake_make_inbound)
m.setattr(channel, "_make_inbound", mock_make_inbound)
channel._on_message(_make_text_event("2", message_id="msg_first"))
channel._on_message(_make_text_event("next", message_id="msg_second"))
first_inbound = created[0]
second_inbound = created[1]
first_metadata = mock_make_inbound.call_args_list[0].kwargs["metadata"]
second_metadata = mock_make_inbound.call_args_list[1].kwargs["metadata"]
assert first_inbound.topic_id == "om_original"
assert second_inbound.topic_id == "msg_second"
assert first_metadata["topic_id"] == "om_original"
assert first_metadata[RESOLVED_FROM_PENDING_CLARIFICATION_METADATA_KEY] is True
assert second_metadata["topic_id"] == "msg_second"
assert second_metadata[RESOLVED_FROM_PENDING_CLARIFICATION_METADATA_KEY] is False
def test_feishu_expired_pending_clarification_is_ignored(monkeypatch):
bus = MessageBus()
channel = FeishuChannel(bus, {"app_id": "test", "app_secret": "test"})
monkeypatch.setattr("app.channels.feishu.time.time", lambda: 10_000.0)
channel._pending_clarifications[channel._pending_key("chat_1", "user_1")] = [_pending("om_original", thread_id="deer-thread-1", card_message_id="om_card", created_at=0.0)]
with pytest.MonkeyPatch.context() as m:
mock_make_inbound = MagicMock()
m.setattr(channel, "_make_inbound", mock_make_inbound)
channel._on_message(_make_text_event("2", message_id="msg_plain_2"))
metadata = mock_make_inbound.call_args.kwargs["metadata"]
assert metadata["topic_id"] == "msg_plain_2"
assert metadata[RESOLVED_FROM_PENDING_CLARIFICATION_METADATA_KEY] is False
assert channel._pending_key("chat_1", "user_1") not in channel._pending_clarifications
def test_feishu_command_does_not_consume_pending_clarification():
bus = MessageBus()
channel = FeishuChannel(bus, {"app_id": "test", "app_secret": "test"})
key = channel._pending_key("chat_1", "user_1")
channel._pending_clarifications[key] = [_pending("om_original", thread_id="deer-thread-1", card_message_id="om_card")]
with pytest.MonkeyPatch.context() as m:
mock_make_inbound = MagicMock()
m.setattr(channel, "_make_inbound", mock_make_inbound)
channel._on_message(_make_text_event("/status", message_id="msg_command"))
metadata = mock_make_inbound.call_args.kwargs["metadata"]
assert mock_make_inbound.call_args.kwargs["msg_type"].value == "command"
assert metadata["topic_id"] == "msg_command"
assert metadata[RESOLVED_FROM_PENDING_CLARIFICATION_METADATA_KEY] is False
assert key in channel._pending_clarifications
def test_feishu_remembers_pending_clarification_only_after_final_card_success():
bus = MessageBus()
channel = FeishuChannel(bus, {"app_id": "test", "app_secret": "test"})
outbound = OutboundMessage(
channel_name="feishu",
chat_id="chat_1",
thread_id="deer-thread-1",
text="clarify?",
thread_ts="om_original",
metadata={
PENDING_CLARIFICATION_METADATA_KEY: True,
"user_id": "user_1",
"topic_id": "om_original",
"message_id": "om_original",
},
)
channel._remember_pending_clarification(outbound, None)
assert channel._pending_clarifications == {}
channel._remember_pending_clarification(outbound, "om_card")
pending = channel._pending_clarifications[channel._pending_key("chat_1", "user_1")][0]
assert pending["topic_id"] == "om_original"
assert pending["thread_id"] == "deer-thread-1"
assert pending["card_message_id"] == "om_card"
def test_feishu_multiple_pending_clarifications_are_consumed_in_order():
bus = MessageBus()
channel = FeishuChannel(bus, {"app_id": "test", "app_secret": "test"})
key = channel._pending_key("chat_1", "user_1")
channel._pending_clarifications[key] = [
_pending("om_first", thread_id="deer-thread-1"),
_pending("om_second", thread_id="deer-thread-2"),
]
with pytest.MonkeyPatch.context() as m:
created = []
def fake_make_inbound(**kwargs):
inbound = InboundMessage(channel_name="feishu", **kwargs)
created.append(inbound)
return inbound
m.setattr(channel, "_make_inbound", MagicMock(side_effect=fake_make_inbound))
channel._on_message(_make_text_event("first answer", message_id="msg_first"))
channel._on_message(_make_text_event("second answer", message_id="msg_second"))
assert [msg.topic_id for msg in created] == ["om_first", "om_second"]
assert key not in channel._pending_clarifications
def test_feishu_explicit_reply_prefers_stored_mapping_over_pending():
bus = MessageBus()
store = ChannelStore(path=Path(tempfile.mkdtemp()) / "store.json")
store.set_thread_id("feishu", "chat_1", "deer-thread-card", topic_id="om_card", user_id="user_1")
channel = FeishuChannel(bus, {"app_id": "test", "app_secret": "test", "channel_store": store})
key = channel._pending_key("chat_1", "user_1")
channel._pending_clarifications[key] = [_pending("om_pending", thread_id="deer-thread-pending")]
with pytest.MonkeyPatch.context() as m:
mock_make_inbound = MagicMock()
m.setattr(channel, "_make_inbound", mock_make_inbound)
channel._on_message(
_make_text_event(
"answer",
message_id="msg_reply",
root_id="om_unknown",
parent_id="om_card",
)
)
metadata = mock_make_inbound.call_args.kwargs["metadata"]
assert metadata["topic_id"] == "om_card"
assert metadata[RESOLVED_FROM_PENDING_CLARIFICATION_METADATA_KEY] is False
assert key in channel._pending_clarifications
@pytest.mark.parametrize("command", sorted(KNOWN_CHANNEL_COMMANDS))
def test_feishu_recognizes_all_known_slash_commands(command):
"""Every entry in KNOWN_CHANNEL_COMMANDS must be classified as a command."""