Browse Source

Say what the checks proved, and check two things they had not

Three fixes where the code claimed more than it had verified:

- thread_root_event_id was checked for existence only, then refused with "it is
  not a thread root of that room" — the one thing never checked. A reply's ID is
  now rejected too, naming the real root so the caller can retry correctly.
- chatto_send_room could resolve a room the bot can see but has not joined
  (ListRooms returns visible channels, membership or room.list permission) and
  fail at post time with a server error naming neither room nor fix. The room
  carries its membership out of the same lookup, and the refusal says what to do.
- _post_chunks caught RuntimeError and reported it as "Chatto API error".
  chattolib maps every transport failure to ChattoError, so anything else is a
  bug here — now labelled as one instead of sending an operator after the server.

Also: drop a dead threading_mode guard (non-optional field), tell the model in
the list_rooms schema that is_member decides postability, and say in
after-install.md that an allowlist entry may be a login or a user ID — the
step that decides whether the bot works at all, and it said only "logins".
Paul Klumpp 2 tuần trước cách đây
mục cha
commit
afc9c21f01
3 tập tin đã thay đổi với 118 bổ sung và 24 xóa
  1. 69 20
      adapter.py
  2. 5 4
      after-install.md
  3. 44 0
      test_adapter.py

+ 69 - 20
adapter.py

@@ -3560,10 +3560,11 @@ CHATTO_LIST_ROOMS_SCHEMA = {
     "name": "chatto_list_rooms",
     "description": (
         "List the Chatto rooms visible to the bot (channels and DM rooms), with "
-        "their room IDs and whether the bot is a member. Use this to resolve a "
-        "channel name to its room ID — e.g. to find the 'platform:chat_id' "
-        "address for a cron job's `deliver` target. Optional `name` filters "
-        "case-insensitively by substring."
+        "their room IDs and whether the bot is a member. Check `is_member` before "
+        "posting: the bot can see rooms it has not joined, and cannot post in "
+        "them. Use this to resolve a channel name to its room ID — e.g. to find "
+        "the 'platform:chat_id' address for a cron job's `deliver` target. "
+        "Optional `name` filters case-insensitively by substring."
     ),
     "parameters": {
         "type": "object",
@@ -3841,7 +3842,9 @@ async def _deliver_chunks(
     ``on_posted`` is the adapter's ``_mark_seen``: the tools have no dedup state
     to keep, the adapter must not treat its own output as unread input. The
     first failure stops the batch and is returned rather than raised — the
-    caller knows better than this function what a partial send means to it.
+    caller knows better than this function what a partial send means to it, and
+    what to call the exception (``ChattoError`` is a server failure, anything
+    else is a bug here).
     """
     normalised = _normalise_outbound_text(message)
     chunks = BasePlatformAdapter.truncate_message(
@@ -3886,8 +3889,17 @@ async def _post_chunks(
         create_thread=create_thread,
     )
     if delivery.error is not None:
+        # Blame the server only for a server error. chattolib maps every
+        # transport failure to ChattoError, so anything else is a bug on our
+        # side — calling that "Chatto API error" would send an operator hunting
+        # the wrong component.
+        cause = (
+            "Chatto API error"
+            if isinstance(delivery.error, ChattoError)
+            else "Internal plugin error"
+        )
         raise _refuse(
-            f"Chatto API error: {delivery.error}",
+            f"{cause}: {delivery.error}",
             # partial marks a half-delivered send; the caller must not
             # resend it from the top without knowing about these chunks.
             partial=bool(delivery.message_ids),
@@ -3969,7 +3981,23 @@ async def _find_dm_room(
     return None
 
 
-async def _resolve_room(client: ChattoClient, room_id: str, room_name: str) -> Room:
+class _ResolvedRoom(NamedTuple):
+    """A room the send path resolved, plus whether the bot belongs to it.
+
+    Membership travels with the room because the directory answers a broader
+    question than the send path can use: a room the bot can *see* is not one it
+    can *post in* (ListRooms returns visible channels, membership or room.list
+    permission). Splitting that into two lookups would be a second chance to
+    disagree with the state the first one already carried.
+    """
+
+    room: Room
+    is_member: bool
+
+
+async def _resolve_room(
+    client: ChattoClient, room_id: str, room_name: str
+) -> _ResolvedRoom:
     """The destination room, by ID or by name.
 
     A name is not unique on Chatto — two room groups may both hold an "Ops" —
@@ -3983,10 +4011,12 @@ async def _resolve_room(client: ChattoClient, room_id: str, room_name: str) -> R
                 f"Room {room_id} was not found, or the bot cannot see it. "
                 "Use chatto_list_rooms to look up a room ID."
             )
-        return room_viewer_state.room
+        return _ResolvedRoom(
+            room_viewer_state.room, room_viewer_state.viewer_state.is_member
+        )
 
     matches = [
-        rws.room
+        rws
         for rws in await client.list_rooms()
         if rws.room is not None
         and (rws.room.name or "").casefold() == room_name.casefold()
@@ -3996,14 +4026,17 @@ async def _resolve_room(client: ChattoClient, room_id: str, room_name: str) -> R
             f"Room not found: {room_name}. Use chatto_list_rooms to see the available names."
         )
     if len(matches) > 1:
-        candidates = ", ".join(f"{room.id} ('{room.name}')" for room in matches)
+        candidates = ", ".join(
+            f"{rws.room.id} ('{rws.room.name}')"  # type: ignore[union-attr]
+            for rws in matches
+        )
         raise _refuse(
             f"Room name '{room_name}' is ambiguous — {len(matches)} rooms carry it: "
             f"{candidates}. Pass the room_id instead."
         )
-    first = matches[0]
-    assert first is not None  # the filter above guarantees a room
-    return first
+    only = matches[0]
+    assert only.room is not None  # the filter above guarantees a room
+    return _ResolvedRoom(only.room, only.viewer_state.is_member)
 
 
 async def _send_dm(args: dict) -> str:
@@ -4068,7 +4101,8 @@ async def _send_room(args: dict) -> str:
 
     cfg = _chatto_config_for_tools()
     async with _tool_client(cfg) as client:
-        room = await _resolve_room(client, room_id, room_name)
+        resolved = await _resolve_room(client, room_id, room_name)
+        room = resolved.room
 
         # Posting into a room the plugin treats as read-only would break a
         # promise the README makes about unlisted rooms staying silent.
@@ -4080,14 +4114,31 @@ async def _send_room(args: dict) -> str:
                 "A DM has no threads: drop create_thread and send it as a normal message."
             )
 
+        # The directory lists rooms the bot can see but has not joined, and
+        # posting into one fails server-side with an error that names neither
+        # the room nor the fix.
+        if not resolved.is_member:
+            raise _refuse(
+                f"Room '{room.name or room.id}' ({room.id}) is not one the bot "
+                "has joined, so it cannot post there. Have someone invite the bot "
+                "natively in Chatto, or send it a direct message, and try again."
+            )
+
         # A thread root has to belong to the room we post into, or the message
-        # attaches to a thread somewhere else entirely.
+        # attaches to a thread somewhere else entirely. And it has to BE a root:
+        # passing a reply's ID would attach to a thread nobody can follow.
         if thread_root_event_id:
             root = await client.get_message(room.id, thread_root_event_id)
             if root is None:
                 raise _refuse(
-                    f"No message {thread_root_event_id} in room {room.id}: it is not "
-                    "a thread root of that room. Send without thread_root_event_id."
+                    f"No message {thread_root_event_id} in room {room.id}. "
+                    "Send without thread_root_event_id to post to the room itself."
+                )
+            if root.thread_root_event_id:
+                raise _refuse(
+                    f"Message {thread_root_event_id} is a reply inside thread "
+                    f"{root.thread_root_event_id}, not a thread root. Pass that "
+                    "thread's root ID, or send without thread_root_event_id."
                 )
 
         message_ids, root_event_id = await _post_chunks(
@@ -4145,9 +4196,7 @@ async def _list_rooms(args: dict) -> str:
                     "room_id": room.id,
                     "name": room.name or "",
                     "kind": room.kind.value,
-                    "threading_mode": room.threading_mode.value
-                    if room.threading_mode
-                    else "",
+                    "threading_mode": room.threading_mode.value,
                     "archived": room.archived,
                     "is_member": rws.viewer_state.is_member,
                     # DM rooms carry no name, so they are labelled by their ID

+ 5 - 4
after-install.md

@@ -51,10 +51,11 @@ can live in either file. Environment variables take precedence over
    CHATTO_ALLOWED_USERS=your-chatto-login,another-login
    ```
 
-   Logins are matched exactly and case-sensitively. To open the bot to every
-   user on the server instead, use `CHATTO_ALLOW_ALL_USERS=true` — which hands
-   all of them the agent's capabilities, including tool use, so keep that for
-   a trusted, private server. Setting both is rejected as conflicting.
+   Each entry is matched exactly and case-sensitively, and an entry may be either
+   a Chatto login or a user ID — whichever you have to hand. To open the bot to
+   every user on the server instead, use `CHATTO_ALLOW_ALL_USERS=true` — which
+   hands all of them the agent's capabilities, including tool use, so keep that
+   for a trusted, private server. Setting both is rejected as conflicting.
 
 4. Restart the Hermes Gateway
 

+ 44 - 0
test_adapter.py

@@ -261,6 +261,7 @@ def _make_message(
     created_at=None,
     updated_at=None,
     actor_id="user-1",
+    thread_root_event_id="",
 ):
     """Build a real chattolib Message, as fetch_message() would return."""
     return Message(
@@ -271,6 +272,7 @@ def _make_message(
         body=body,
         attachments=list(attachments or []),
         updated_at=updated_at,
+        thread_root_event_id=thread_root_event_id,
     )
 
 
@@ -3831,6 +3833,48 @@ class TestPluginToolHandlers:
         assert result["success"] is False
         client.post_message.assert_not_awaited()
 
+    async def test_send_room_refuses_a_reply_as_a_thread_root(self):
+        """A thread_root_event_id must be a thread's root. Passing the ID of a
+        reply attaches to a thread nobody follows, and the bot would post into
+        it silently.
+        """
+        room = _make_room("room-1", "Ops", RoomKind.CHANNEL)
+        client = self._stub_client()
+        client.get_room = AsyncMock(return_value=_make_room_state(room))
+        client.get_message = AsyncMock(
+            return_value=_make_message(
+                message_id="msg-9", room_id="room-1", thread_root_event_id="msg-1"
+            )
+        )
+        result, _, _ = await self._run(
+            _handle_chatto_send_room,
+            {"room_id": "room-1", "message": "hi", "thread_root_event_id": "msg-9"},
+            client=client,
+            optional_mention_rooms="room-1",
+        )
+        assert result["success"] is False
+        assert "msg-1" in result["error"]
+        client.post_message.assert_not_awaited()
+
+    async def test_send_room_refuses_a_room_the_bot_has_not_joined(self):
+        """The directory lists rooms the bot can see but has not joined, and
+        posting into one fails server-side with an error naming neither the
+        room nor the fix."""
+        room = _make_room("room-1", "Ops", RoomKind.CHANNEL)
+        client = self._stub_client()
+        client.get_room = AsyncMock(
+            return_value=_make_room_state(room, is_member=False)
+        )
+        result, _, _ = await self._run(
+            _handle_chatto_send_room,
+            {"room_id": "room-1", "message": "hi"},
+            client=client,
+            optional_mention_rooms="room-1",
+        )
+        assert result["success"] is False
+        assert "has joined" in result["error"]
+        client.post_message.assert_not_awaited()
+
     async def test_send_room_refuses_a_thread_root_from_another_room(self):
         """A stale or foreign root must not silently attach the message to a
         thread somewhere else."""