diff --git a/src/openjarvis/channels/_stubs.py b/src/openjarvis/channels/_stubs.py index 6b82e6a8..e1c4eb4a 100644 --- a/src/openjarvis/channels/_stubs.py +++ b/src/openjarvis/channels/_stubs.py @@ -60,7 +60,27 @@ class BaseChannel(ABC): conversation_id: str = "", metadata: Dict[str, Any] | None = None, ) -> bool: - """Send a message to a specific channel. Returns True on success.""" + """Send a message to a specific channel. Returns True on success. + + Canonical send contract shared by **every** channel adapter: + + ``channel`` + The DESTINATION identifier — the per-adapter native id of the + place the message goes (Discord/Slack channel id, Telegram chat + id, email recipient address, ...). This is *not* the channel + TYPE label. An incoming :class:`ChannelMessage` carries that + destination in its ``conversation_id`` field (``channel`` there + is only the type label such as ``"discord"``), so dispatch code + replying to a message must pass ``cm.conversation_id`` here. + ``conversation_id`` + An optional reply/thread reference — the native id of the + message being replied to (Discord ``message_reference``, Slack + ``thread_ts``, Telegram ``reply_to_message_id``, email + ``In-Reply-To``, ...). When replying to an inbound message this + should be ``cm.message_id``, never the channel id. Passing a + channel id here yields broken references (e.g. Discord + ``MESSAGE_REFERENCE_UNKNOWN_MESSAGE``). + """ @abstractmethod def status(self) -> ChannelStatus: diff --git a/src/openjarvis/channels/telegram.py b/src/openjarvis/channels/telegram.py index a620be17..37522bfe 100644 --- a/src/openjarvis/channels/telegram.py +++ b/src/openjarvis/channels/telegram.py @@ -112,7 +112,15 @@ class TelegramChannel(BaseChannel): _TELEGRAM_MAX_LEN = 4096 url = f"https://api.telegram.org/bot{self._token}/sendMessage" - chat_id = conversation_id or channel + # Canonical channel send contract (see BaseChannel.send): the first + # positional ``channel`` arg is the DESTINATION (the Telegram chat + # id). ``conversation_id`` is the inbound message id used as a + # reply/thread reference (``reply_to_message_id``). We fall back to + # ``conversation_id`` as the chat id only when ``channel`` is empty, + # for backwards compatibility with legacy callers that passed the + # chat id via ``conversation_id``. + chat_id = channel or conversation_id + reply_to = conversation_id if (channel and conversation_id) else "" chunks = textwrap.wrap( content, width=_TELEGRAM_MAX_LEN, @@ -126,6 +134,8 @@ class TelegramChannel(BaseChannel): } if self._parse_mode: payload["parse_mode"] = self._parse_mode + if reply_to: + payload["reply_to_message_id"] = reply_to resp = httpx.post(url, json=payload, timeout=10.0) if resp.status_code >= 300: diff --git a/src/openjarvis/system/core.py b/src/openjarvis/system/core.py index bfb94ed8..95f48179 100644 --- a/src/openjarvis/system/core.py +++ b/src/openjarvis/system/core.py @@ -268,10 +268,20 @@ class JarvisSystem: if reply: try: + # Canonical channel send contract (see BaseChannel.send): + # the first positional arg is the DESTINATION id, and the + # `conversation_id=` kwarg is the inbound message id used as + # a reply/thread reference. ``cm.conversation_id`` holds the + # real per-adapter destination (Discord/Slack channel id, + # Telegram chat id, ...) while ``cm.channel`` is only the + # channel TYPE label ("discord", "telegram", ...). Passing + # the type label as the destination produced HTTP 400s + # (#515) and using the channel id as a reply reference + # produced MESSAGE_REFERENCE_UNKNOWN_MESSAGE (#516). channel_bridge.send( - cm.channel, + cm.conversation_id, reply, - conversation_id=cm.conversation_id, + conversation_id=getattr(cm, "message_id", ""), ) except Exception: logger.exception("Channel send error") diff --git a/tests/channels/test_discord_channel.py b/tests/channels/test_discord_channel.py index 5b684d7f..b4631068 100644 --- a/tests/channels/test_discord_channel.py +++ b/tests/channels/test_discord_channel.py @@ -130,3 +130,63 @@ class TestStatus: ch = DiscordChannel() ch.connect() assert ch.status() == ChannelStatus.ERROR + + +class TestWireChannelEndToEnd: + """Regression for #515/#516 — the full inbound→reply path through + JarvisSystem.wire_channel must call the real Discord REST API with the + numeric channel id (not "discord") and a message_reference equal to the + inbound message id (not the channel id). + """ + + def test_reply_hits_real_channel_id_and_message_reference(self, tmp_path): + from openjarvis.channels._stubs import ChannelMessage + from openjarvis.core.config import JarvisConfig + from openjarvis.core.events import EventBus + from openjarvis.system import JarvisSystem + + config = JarvisConfig() + config.sessions.db_path = str(tmp_path / "sessions.db") + from unittest.mock import MagicMock as _MM + + system = JarvisSystem( + config=config, + bus=EventBus(record_history=False), + engine=_MM(), + engine_key="mock", + model="test-model", + agent_name="", + ) + system.ask = _MM(return_value={"content": "pong"}) + + channel = DiscordChannel(bot_token="my-bot-token") + system.wire_channel(channel) + + # Exactly the ChannelMessage shape DiscordChannel._gateway_loop emits: + # channel = "discord" (TYPE label), conversation_id = numeric channel + # id, message_id = numeric message id. + cm = ChannelMessage( + channel="discord", + sender="user-1", + content="hello", + message_id="111122223333444455", + conversation_id="987654321098765432", + ) + + mock_response = MagicMock() + mock_response.status_code = 200 + with patch("httpx.post", return_value=mock_response) as mock_post: + # Invoke the handler wire_channel registered on the channel. + for handler in channel._handlers: + handler(cm) + + mock_post.assert_called_once() + url = mock_post.call_args[0][0] + # #515: destination is the numeric channel id, not the "discord" label. + assert "discord.com/api/v10/channels/987654321098765432/messages" in url + assert "channels/discord/messages" not in url + payload = mock_post.call_args[1]["json"] + assert payload["content"] == "pong" + # #516: message_reference is the inbound message id, NOT the channel id. + assert payload["message_reference"] == {"message_id": "111122223333444455"} + assert payload["message_reference"]["message_id"] != "987654321098765432" diff --git a/tests/channels/test_telegram.py b/tests/channels/test_telegram.py index ce8f31c2..e87cf396 100644 --- a/tests/channels/test_telegram.py +++ b/tests/channels/test_telegram.py @@ -105,6 +105,41 @@ class TestSend: event_types = [e.event_type for e in bus.history] assert EventType.CHANNEL_MESSAGE_SENT in event_types + def test_send_uses_channel_as_chat_id_under_unified_contract(self): + """Canonical contract (#515/#516): the first positional ``channel`` + arg is the chat destination, and ``conversation_id`` is the inbound + message id used as ``reply_to_message_id`` — not the chat id.""" + ch = TelegramChannel(bot_token="123:ABC") + + mock_response = MagicMock() + mock_response.status_code = 200 + + with patch("httpx.post", return_value=mock_response) as mock_post: + result = ch.send("12345678", "Reply!", conversation_id="55") + assert result is True + payload = mock_post.call_args[1]["json"] + # Destination is the chat id from the positional channel arg. + assert payload["chat_id"] == "12345678" + # conversation_id becomes the reply reference, not the chat id. + assert payload["reply_to_message_id"] == "55" + + def test_send_legacy_conversation_id_only_still_targets_chat(self): + """Backwards compatibility: a legacy caller passing the chat id via + ``conversation_id`` (with an empty ``channel``) still delivers.""" + ch = TelegramChannel(bot_token="123:ABC") + + mock_response = MagicMock() + mock_response.status_code = 200 + + with patch("httpx.post", return_value=mock_response) as mock_post: + result = ch.send("", "Hello!", conversation_id="12345678") + assert result is True + payload = mock_post.call_args[1]["json"] + assert payload["chat_id"] == "12345678" + # When channel is empty, conversation_id is the chat id, so it must + # not also be used as a self-referential reply id. + assert "reply_to_message_id" not in payload + class TestStatus: def test_no_token_connect_error(self): diff --git a/tests/cli/test_serve_channel_wiring.py b/tests/cli/test_serve_channel_wiring.py index bf199d8e..b0cc1946 100644 --- a/tests/cli/test_serve_channel_wiring.py +++ b/tests/cli/test_serve_channel_wiring.py @@ -79,10 +79,14 @@ class TestWireChannelWithAgent: system.ask.assert_called_once() assert system.ask.call_args[0][0] == "ping" + # Canonical send contract (#515/#516): the destination is the real + # per-adapter id (carried in ChannelMessage.conversation_id), not the + # channel TYPE label, and the conversation_id kwarg is the inbound + # message id used as a reply reference, not the channel id. mock_channel.send.assert_called_once_with( - "telegram", + "42", "pong", - conversation_id="42", + conversation_id="1", ) def test_session_store_created_lazily(self, tmp_path): @@ -126,13 +130,101 @@ class TestWireChannelWithEngine: handler = mock_channel.on_message.call_args[0][0] handler(_make_channel_message(content="hi")) + # Canonical send contract (#515/#516): destination = real channel id + # (from ChannelMessage.conversation_id), reply ref = inbound message id. mock_channel.send.assert_called_once_with( - "telegram", + "42", "raw reply", - conversation_id="42", + conversation_id="1", ) +class TestWireChannelCanonicalContract: + """Regression for #515/#516 — wire_channel must dispatch the canonical + send contract so each adapter receives the right destination/reply ids, + regardless of the channel TYPE label. + """ + + def test_discord_uses_real_channel_id_not_type_label(self, tmp_path): + """A Discord ChannelMessage (channel="discord" TYPE label, + conversation_id=, message_id=) + must reply to the numeric channel id, with the message id as the + reply reference — never "discord" as destination (#515) and never the + channel id as the message reference (#516). + """ + system = _make_system(tmp_path=tmp_path) + system.ask = MagicMock(return_value={"content": "pong"}) + + mock_channel = MagicMock() + system.wire_channel(mock_channel) + handler = mock_channel.on_message.call_args[0][0] + + cm = ChannelMessage( + channel="discord", + sender="user-1", + content="hello", + message_id="111122223333444455", + conversation_id="987654321098765432", + ) + handler(cm) + + args, kwargs = mock_channel.send.call_args + # Destination is the real Discord channel id, NOT the type label. + assert args[0] == "987654321098765432" + assert args[0] != "discord" + # Reply reference is the inbound message id, NOT the channel id. + assert kwargs["conversation_id"] == "111122223333444455" + assert kwargs["conversation_id"] != "987654321098765432" + + def test_telegram_uses_chat_id_and_message_id(self, tmp_path): + """Telegram must keep working under the unified contract: destination + is the chat id (conversation_id), reply ref is the message id.""" + system = _make_system(tmp_path=tmp_path) + system.ask = MagicMock(return_value={"content": "pong"}) + + mock_channel = MagicMock() + system.wire_channel(mock_channel) + handler = mock_channel.on_message.call_args[0][0] + + cm = ChannelMessage( + channel="telegram", + sender="42", + content="ping", + message_id="55", + conversation_id="12345678", + ) + handler(cm) + + mock_channel.send.assert_called_once_with( + "12345678", + "pong", + conversation_id="55", + ) + + def test_session_key_still_uses_conversation_id(self, tmp_path): + """The fix must not change session isolation keying, which still uses + ``:``.""" + system = _make_system(tmp_path=tmp_path) + system.ask = MagicMock(return_value={"content": "ok"}) + + mock_channel = MagicMock() + system.wire_channel(mock_channel) + handler = mock_channel.on_message.call_args[0][0] + + cm = ChannelMessage( + channel="discord", + sender="u1", + content="hi", + message_id="msg-9", + conversation_id="chan-7", + ) + handler(cm) + + # Session was created under the channel:conversation_id key. + session = system.session_store.get_or_create("discord:chan-7") + assert any(m.content == "hi" for m in session.messages) + + class TestWireChannelSessionIsolation: """Separate conversation_ids get independent sessions."""