fix(channels): unify send() destination/reply contract (Discord #515/#516) (#528)

The channel `send()` contract was inconsistent across adapters. Almost
every adapter (Discord, Slack, email, WhatsApp, ...) treats the first
positional `channel` arg as the real DESTINATION id and `conversation_id`
as an optional reply/thread reference. Telegram alone treated
`conversation_id` as the destination (`chat_id = conversation_id or
channel`).

`JarvisSystem._on_channel_message` hard-coded the Telegram-shaped mapping
for ALL channels: `send(cm.channel, reply, conversation_id=cm.conversation_id)`.
Since inbound `ChannelMessage`s carry the channel TYPE label in `.channel`
("discord") and the real destination id in `.conversation_id`, this sent
the literal "discord" as the Discord channel id (HTTP 400
NUMBER_TYPE_COERCE, #515) and passed the channel id as a Discord
`message_reference` (MESSAGE_REFERENCE_UNKNOWN_MESSAGE, #516).

Fix: define and document ONE canonical contract on `BaseChannel.send` —
positional `channel` = destination id, `conversation_id` = inbound message
id used as a reply reference — and dispatch it from `_on_channel_message`
as `send(cm.conversation_id, reply, conversation_id=cm.message_id)`,
matching the already-fixed `ChannelAgent` path (#495/#459). Telegram's
`send()` is brought into line (destination = `channel`, with a
`reply_to_message_id` reply ref and a legacy `conversation_id`-only
fallback) so it keeps working unchanged.

The DiscordChannel `_gateway_loop` ChannelMessage shape locked by #495 is
untouched; `tests/agents/test_channel_agent.py` passes unchanged. The
stale `test_serve_channel_wiring.py` assertions (which encoded the old
buggy mapping from #94) are updated, and regression tests are added for
Discord (real channel id + correct message_reference), Telegram (chat id
+ reply ref + legacy fallback), and per-channel dispatch in
`_on_channel_message`.

Fixes #515
Fixes #516

Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com>

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:

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:

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")

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"

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):

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=<numeric channel id>, message_id=<numeric msg 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
+        ``<channel>:<conversation_id>``."""
+        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."""