Sfoglia il codice sorgente

Page chatto_get_history with cursors, and refuse unjoined rooms

Two holes in the read tools, both found by reading the Chatto API docs
rather than the code.

`has_older` told the model that older history existed while giving it no
way to reach it, so a room with 500 messages answered "what did we decide
in June" with the 20 newest ones and a shrug. The server has had the
missing half all along: `cursor.before` and `cursor.after` page a timeline
from an opaque position, and chattolib passes both through. The tool now
takes `before`/`after` and answers with `start_cursor`, `end_cursor` and
`has_newer` beside `has_older`, so the next page is one call away with
nothing to guess. Both cursors together are refused rather than merged:
chattolib sends `before` OR `after` and drops the second without a word,
and a silently narrowed page is worse than a refusal.

The read tools also claimed the bot could read any room it could see.
Chatto documents room membership as a precondition on every read the
plugin makes — GetRoomEvents, GetMessage and BatchGetMessages — so that
sentence was wrong in README, AGENTS.md and SKILL.md alike. Both read
tools now check membership up front and refuse with the same /join
remedy the send tool uses, since a bare PERMISSION_DENIED names neither
the room nor the way in. _not_joined_refusal grew an `action` argument
rather than becoming a second copy with different wording.

A search tool stays unwritten on purpose: Chatto has MessageSearchService,
but it exists only on unreleased main and chattolib 0.5.0b17 ships no such
client, and hand-rolling the RPC is what vendoring chattolib exists to
avoid. It returns once upstream releases a build that has it.
Paul Klumpp 1 settimana fa
parent
commit
370574530e
5 ha cambiato i file con 224 aggiunte e 24 eliminazioni
  1. 14 7
      AGENTS.md
  2. 3 3
      README.md
  3. 62 10
      adapter.py
  4. 4 2
      skills/chatto-platform-plugin/SKILL.md
  5. 141 2
      test_adapter.py

+ 14 - 7
AGENTS.md

@@ -226,9 +226,12 @@ changes must keep them, or consciously renegotiate the docs.**
   tools obey the same lists: `chatto_send_room` refuses a channel on neither
   (a DM room is never gated), because a tool that posted there would break the
   read-only promise the same paragraph makes. The read tools are deliberately
-  exempt — `chatto_get_message` and `chatto_get_history` post nothing and
-  answer nothing, so reading a room grants no privilege the bot's token did not
-  already have and the server's own visibility rules are the only gate.
+  exempt from *that list* — `chatto_get_message` and `chatto_get_history` post
+  nothing and answer nothing, so the mention lists have nothing to say about
+  them — but not from membership: every read RPC the plugin uses documents
+  room membership as a precondition (`GetRoomEvents`, `GetMessage`,
+  `BatchGetMessages`), so both refuse an unjoined room through
+  `_not_joined_refusal`, which names what the caller wanted to do there.
   `CHATTO_HOME_CHANNEL` is orthogonal: it is the default outbound target for
   context-less cron/notification delivery, not an answer destination.
 - **Threads stay threads.** An inbound thread reply keeps its thread context;
@@ -544,10 +547,14 @@ constants, not on shared code.
 shape appears wherever two tools answer the same question about the same event:
 **`_resolve_thread_root`** decides "is this event ID a root of a thread in *this*
 room" for both tools taking a `thread_root_event_id` (`_send_room` posting into
-one, `_get_history` reading one). Only one of them sends, so this rule does not
-belong to the pair above — but the drift it prevents is identical: two copies
-would leave two tools disagreeing about the same event ID, and the caller has no
-way to tell which one to believe.
+one, `_get_history` reading one), and **`_not_joined_refusal`** answers "may the
+bot act here at all" for all three (`_send_room` posting, `_get_message` and
+`_get_history` reading — every read RPC documents room membership as a
+precondition, so reading is not a way around the send gate). Only one of the
+first pair sends, so neither rule belongs to the pair above — but the drift they
+prevent is identical: two copies would leave two tools disagreeing about the
+same room or the same event ID, and the caller has no way to tell which one to
+believe.
 
 **State questions: check what exists before adding.** The sweep above catches
 dead state after the fact; the cheaper move is to not create parallel state

+ 3 - 3
README.md

@@ -387,14 +387,14 @@ Beyond answering inbound messages, the agent can act on Chatto on its own initia
 | `chatto_send_room` | Posts a message into a channel or DM room, by `room_id` or `room_name`. Can post inside an existing thread (`thread_root_event_id`) or open a new one (`create_thread`). Either way the bot follows the thread afterwards, so later replies in it reach the agent. |
 | `chatto_send_dm` | Posts a message into the DM room you already have with a person, addressed by `user_id` or `user_login`. |
 | `chatto_get_message` | Reads one message (or several, via `event_ids`, answered in the order you list them) with all its data: body, author, timestamps, reply and thread references, attachments, reactions, pinned and deleted status. `follow_reply_chain` (0-10) additionally resolves the messages that message replies to, oldest-first — use it to resolve a `reply_to=` reference. It works with a single `event_id` only, because a chain belongs to one message. |
-| `chatto_get_history` | Reads the most recent messages of a room (or of one thread, via `thread_root_event_id`), oldest-first, in a page of up to `limit` messages (1-100, default 20). The page is always the newest one — `has_older` tells you a room holds more history than this reaches, but there is no way to ask for the page before it. |
+| `chatto_get_history` | Reads the most recent messages of a room (or of one thread, via `thread_root_event_id`), oldest-first, in a page of up to `limit` messages (1-100, default 20). To read further back, pass the previous answer's `start_cursor` as `before` (or `end_cursor` as `after`); `has_older` tells you whether there is anything left to page to. One call reads one page. |
 
 Four boundaries are deliberate, and each tool reports them as an error instead of half-doing the job:
 
-- **A channel must be opted in.** `chatto_send_room` only posts into a room listed in `CHATTO_REQUIRE_MENTION_ROOMS` or `CHATTO_OPTIONAL_MENTION_ROOMS` (DM rooms are never gated). A room on neither list stays read-only — a human message there would never have produced output either. The refusal names both settings so you can paste the room ID into `~/.hermes/.env`. The read tools are exempt: `chatto_get_message` and `chatto_get_history` read any room the server lets the bot see, because reading answers nothing and posts nothing — the server's own visibility rules are the only gate.
+- **A channel must be opted in.** `chatto_send_room` only posts into a room listed in `CHATTO_REQUIRE_MENTION_ROOMS` or `CHATTO_OPTIONAL_MENTION_ROOMS` (DM rooms are never gated). A room on neither list stays read-only — a human message there would never have produced output either. The refusal names both settings so you can paste the room ID into `~/.hermes/.env`. The read tools are exempt from that rule: `chatto_get_message` and `chatto_get_history` read any room the bot has joined, listed or not — they post nothing and answer nothing, so the mention lists have nothing to say about them.
 - **The bot cannot open a DM.** Only a person can start a direct message on Chatto, so `chatto_send_dm` needs a recipient who has written to the bot at least once. To reach someone else, mention them in a channel message.
 - **A name is not an identity.** If several rooms share the name you passed, `chatto_send_room` returns their IDs and asks for `room_id` instead of picking one. Same rule as `/join #name`. The read tools resolve names by the same rule.
-- **Visible is not joined.** `chatto_list_rooms` lists rooms the bot can see, which includes channels it has not joined; check `is_member` before posting into one. `chatto_send_room` refuses an unjoined room and says how to get in, because the server would reject the post with an error naming neither the room nor the fix. Reading needs no membership beyond what the server allows — a room the bot merely sees stays readable if the server permits the timeline.
+- **Visible is not joined.** `chatto_list_rooms` lists rooms the bot can see, which includes channels it has not joined; check `is_member` before posting into one. `chatto_send_room` refuses an unjoined room and says how to get in, because the server would reject the post with an error naming neither the room nor the fix. Reading needs membership too — Chatto requires it for every read — so the read tools refuse an unjoined room the same way.
 
 Credentials never come from the model: no tool takes a server URL or a token as an argument, so the configured bot token cannot be pointed at another host. Inside a channel, mention people in the message text (`@login, …`) — there is no separate mention parameter.
 

+ 62 - 10
adapter.py

@@ -4113,7 +4113,8 @@ CHATTO_GET_HISTORY_SCHEMA = {
         "the thread's events are read instead of the room timeline; the root "
         "has to belong to the room and BE a root, like in chatto_send_room. "
         "`limit` (1-100, default 20) bounds the page; `has_older` says whether "
-        "older history exists."
+        "older history exists. To read further back, pass the previous "
+        "result's `start_cursor` as `before` — one call reads one page."
     ),
     "parameters": {
         "type": "object",
@@ -4146,6 +4147,21 @@ CHATTO_GET_HISTORY_SCHEMA = {
                     "Maximum number of messages to return, 1-100 (default 20)."
                 ),
             },
+            "before": {
+                "type": "string",
+                "description": (
+                    "Optional: page backwards. Pass the `start_cursor` of the "
+                    "previous result — cursors are opaque positions, not event "
+                    "IDs. Mutually exclusive with `after`."
+                ),
+            },
+            "after": {
+                "type": "string",
+                "description": (
+                    "Optional: page forwards. Pass the `end_cursor` of the "
+                    "previous result. Mutually exclusive with `before`."
+                ),
+            },
         },
         "required": [],
         "additionalProperties": False,
@@ -4273,13 +4289,15 @@ def _room_is_writable(cfg: ChattoConfiguration | None, room: Room) -> bool:
     return _room_answers_in(cfg, room.id, room.kind)
 
 
-def _not_joined_refusal(room: Room) -> _ToolRefusal:
+def _not_joined_refusal(room: Room, action: str) -> _ToolRefusal:
     """The refusal for a room the bot can see but has not joined.
 
-    The room kinds need different advice, and giving a channel the DM remedy
-    sends the user down a path that cannot work: a direct message to the bot is
-    not how you get it into a channel. For a channel the paths are an invite in
-    Chatto, or the ``/join`` the bot answers over DM.
+    ``action`` names what the caller wanted to do there — "post there", "read
+    its history there" — because the remedy is identical for both while the
+    symptom is not, and a read that reports a sending problem sends the reader
+    looking in the wrong place. The room kinds differ in the remedy: giving a
+    channel the DM remedy sends the user down a path that cannot work, since a
+    direct message to the bot is not how you get it into a channel.
 
     The test is on ``DM`` rather than on ``CHANNEL`` on purpose: a server that
     never sets ``kind`` reports ``UNSPECIFIED``, and this plugin counts an unknown
@@ -4294,7 +4312,7 @@ def _not_joined_refusal(room: Room) -> _ToolRefusal:
     )
     return _refuse(
         f"Room '{room.name or room.id}' ({room.id}) is not one the bot has "
-        f"joined, so it cannot post there. {remedy}, then try again."
+        f"joined, so it cannot {action}. {remedy}, then try again."
     )
 
 
@@ -4653,7 +4671,7 @@ async def _send_room(args: dict) -> str:
         # posting into one fails server-side with an error that names neither
         # the room nor the fix.
         if not resolved.is_member:
-            raise _not_joined_refusal(room)
+            raise _not_joined_refusal(room, "post there")
 
         # A thread root has to belong to this room and has to BE a root, or
         # the message attaches to a thread somewhere else entirely — or to
@@ -4893,6 +4911,13 @@ async def _get_message(args: dict) -> str:
         resolved = await _resolve_room(client, room_id, room_name)
         room = resolved.room
 
+        # Every read RPC the plugin uses documents room membership as a
+        # precondition (GetRoomEvents, GetMessage, BatchGetMessages), so an
+        # unjoined room is unreadable whatever the mention lists say. Saying so
+        # here beats a PERMISSION_DENIED that names neither the room nor the fix.
+        if not resolved.is_member:
+            raise _not_joined_refusal(room, "read it there")
+
         if event_id:
             # One message is one lookup: the single-message RPC is the
             # authoritative one, so there is no batch to fall back from.
@@ -4951,22 +4976,42 @@ async def _get_history(args: dict) -> str:
     room_name = args.get("room_name") or ""
     thread_root_event_id = args.get("thread_root_event_id") or ""
     limit = _int_arg(args, "limit", HISTORY_PAGE_SIZE, low=1, high=HISTORY_PAGE_MAX)
+    before = args.get("before") or ""
+    after = args.get("after") or ""
 
     if not room_id and not room_name:
         raise _refuse("Exactly one of room_id or room_name is required")
+    if before and after:
+        # The client sends `before` OR `after` and drops the second without a
+        # word, so both would answer a question nobody asked.
+        raise _refuse(
+            "before and after are mutually exclusive — pick the direction you "
+            "want to page in"
+        )
 
     async with _tool_client(_chatto_config_for_tools()) as client:
         resolved = await _resolve_room(client, room_id, room_name)
         room = resolved.room
 
+        # A read of an unjoined room fails server-side whatever the mention
+        # lists say; saying so here names the room and the way in.
+        if not resolved.is_member:
+            raise _not_joined_refusal(room, "read its history there")
+
         root: Message | None = None
         if thread_root_event_id:
             root = await _resolve_thread_root(client, room, thread_root_event_id)
             page = await client.get_thread_events(
-                room.id, thread_root_event_id, limit=limit
+                room.id,
+                thread_root_event_id,
+                limit=limit,
+                before=before or None,
+                after=after or None,
             )
         else:
-            page = await client.get_room_events(room.id, limit=limit)
+            page = await client.get_room_events(
+                room.id, limit=limit, before=before or None, after=after or None
+            )
 
         messages = [ev.message for ev in page.events if ev.message is not None]
         if root is not None and all(m.id != root.id for m in messages):
@@ -5004,6 +5049,13 @@ async def _get_history(args: dict) -> str:
                 "thread_root_event_id": thread_root_event_id or None,
                 "count": len(messages),
                 "has_older": page.has_older,
+                "has_newer": page.has_newer,
+                # Paging is the caller's next call, so this answer has to carry
+                # the positions that next call takes as its arguments —
+                # otherwise the model would have to guess which message to
+                # anchor on.
+                "start_cursor": page.start_cursor,
+                "end_cursor": page.end_cursor,
                 "messages": [_message_out(m, author_logins) for m in messages],
             }
         )

+ 4 - 2
skills/chatto-platform-plugin/SKILL.md

@@ -50,11 +50,13 @@ Resolves the room (same `_resolve_room` as the send tools) and fetches the messa
 
 - `follow_reply_chain` (0-10): walks the `in_reply_to` chain backwards (`client.get_message` per hop) and returns the ancestors oldest-first in `reply_chain`. A single `event_id` only — a chain belongs to one message, so the tool refuses the combination rather than picking one. A hop the server cannot resolve ends the chain.
 - `event_ids[]`: batch fetch, answered in the order asked for; unknown IDs come back in `missing_event_ids` instead of failing the whole call. A single unknown `event_id` refuses cleanly.
-- **Not gated on the mention lists**: reading posts nothing and answers nothing, so any room the server lets the bot see is readable — the server's visibility rules are the only gate.
+- **Not gated on the mention lists**: reading posts nothing and answers nothing, so any joined room is readable, listed or not. Room membership still applies — the server requires it for every read — and an unjoined room is refused with the `/join` remedy rather than a raw `PERMISSION_DENIED`.
 
 ### chatto_get_history(room_id|room_name, thread_root_event_id?, limit?) — read recent context
 
-Reads the newest messages of a room (`client.get_room_events`) or of one thread (`client.get_thread_events`), returned oldest-first (`_oldest_first`, the same ordering the thread seeding uses — the server's own ordering is not a contract). `limit` 1-100 (default 20), `has_older` in the result says whether more history exists — but there is no `before` argument, so the newest page is the only one reachable (`client.get_room_events` takes one; adding it to the schema is the whole change). Author logins come from the timeline page's own `users_by_id` directory first; actors missing there are hydrated via `batch_get_room_members`. Same room resolution and the same thread-root rule as `chatto_send_room` — both go through `_resolve_thread_root`, so the two tools cannot disagree about what a root is; a thread page that omits its root gets it prepended (the root is already in hand from that check). Not gated on the mention lists either — the server's own visibility rules decide what the bot may read.
+Reads the newest messages of a room (`client.get_room_events`) or of one thread (`client.get_thread_events`), returned oldest-first (`_oldest_first`, the same ordering the thread seeding uses — the server's own ordering is not a contract). `limit` 1-100 (default 20). Paging: `before` (a previous result's `start_cursor`) walks towards older events, `after` (an `end_cursor`) towards newer ones; the cursors are opaque positions, NOT event IDs, and the two are mutually exclusive because chattolib sends `before` OR `after` and would drop the second silently — one call reads one page, and the answer carries `start_cursor`/`end_cursor`/`has_older`/`has_newer` so the next call needs nothing else. Author logins come from the timeline page's own `users_by_id` directory first; actors missing there are hydrated via `batch_get_room_members`. Same room resolution and the same thread-root rule as `chatto_send_room` — both go through `_resolve_thread_root`, so the two tools cannot disagree about what a root is; a thread page that omits its root gets it prepended (the root is already in hand from that check).
+
+Not gated on the mention lists — but room membership is still required (`GetRoomEvents` says so, and so do `GetMessage`/`BatchGetMessages`), so both read tools refuse an unjoined room via `_not_joined_refusal` rather than handing the model a bare `PERMISSION_DENIED`.
 
 ### chatto_send_dm(user_login|user_id, message) — DM a user
 

+ 141 - 2
test_adapter.py

@@ -5422,8 +5422,9 @@ class TestPluginToolHandlers:
 
     async def test_get_history_reads_a_room_the_send_tool_would_refuse(self):
         """Reading breaks no read-only promise — the send gate exists for
-        posting, and a visible room's history stays readable without the
-        mention lists."""
+        posting, and a joined room's history stays readable without the
+        mention lists. Membership is a separate requirement the read tools
+        share with the send one (see the refusal below)."""
         room = _make_room("room-9", "Lurk", RoomKind.CHANNEL)
         client = self._stub_client()
         client.get_room = AsyncMock(return_value=_make_room_state(room))
@@ -5437,6 +5438,144 @@ class TestPluginToolHandlers:
         )
         assert result["success"] is True
 
+    async def test_get_history_pages_backwards_with_a_cursor(self):
+        """`before` is a start_cursor from the previous page, not an event ID:
+        the server walks the timeline from an opaque position. One call reads
+        one page — paging is the caller's next call, not a loop inside this one.
+        """
+        room = self._readable_room()
+        client = self._stub_client()
+        client.get_room = AsyncMock(return_value=_make_room_state(room))
+        page = self._timeline_page([_make_message(message_id="msg-1")])
+        page.start_cursor = "cur-start"
+        page.end_cursor = "cur-end"
+        page.has_older = True
+        page.has_newer = True
+        client.get_room_events = AsyncMock(return_value=page)
+        result, _, _ = await self._run(
+            _handle_chatto_get_history,
+            {"room_id": "room-1", "before": "cur-start"},
+            client=client,
+        )
+        assert result["success"] is True
+        assert client.get_room_events.await_args.kwargs["before"] == "cur-start"
+        # The next page is one call away only if this answer carries what it
+        # takes to make that call.
+        assert result["start_cursor"] == "cur-start"
+        assert result["end_cursor"] == "cur-end"
+        assert result["has_older"] is True
+        assert result["has_newer"] is True
+
+    async def test_get_history_pages_forwards_with_a_cursor(self):
+        room = self._readable_room()
+        client = self._stub_client()
+        client.get_room = AsyncMock(return_value=_make_room_state(room))
+        client.get_room_events = AsyncMock(
+            return_value=self._timeline_page([_make_message(message_id="msg-2")])
+        )
+        result, _, _ = await self._run(
+            _handle_chatto_get_history,
+            {"room_id": "room-1", "after": "cur-end"},
+            client=client,
+        )
+        assert result["success"] is True
+        assert client.get_room_events.await_args.kwargs["after"] == "cur-end"
+
+    async def test_get_history_cursors_reach_a_thread_read_too(self):
+        """One timeline, one rule: a thread page pages the same way a room page
+        does, or a long thread goes unreachable while a room does not."""
+        room = self._readable_room()
+        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-1", body="root")
+        )
+        client.get_thread_events = AsyncMock(
+            return_value=self._timeline_page([_make_message(message_id="msg-1")])
+        )
+        result, _, _ = await self._run(
+            _handle_chatto_get_history,
+            {
+                "room_id": "room-1",
+                "thread_root_event_id": "msg-1",
+                "before": "cur-start",
+            },
+            client=client,
+        )
+        assert result["success"] is True
+        assert client.get_thread_events.await_args.kwargs["before"] == "cur-start"
+
+    async def test_get_history_refuses_both_cursors_at_once(self):
+        """chattolib sends `before` OR `after`, never both — the second is
+        dropped without a word. Refusing beats handing back a page that is not
+        the one that was asked for."""
+        room = self._readable_room()
+        client = self._stub_client()
+        client.get_room = AsyncMock(return_value=_make_room_state(room))
+        client.get_room_events = AsyncMock()
+        result, _, _ = await self._run(
+            _handle_chatto_get_history,
+            {"room_id": "room-1", "before": "a", "after": "b"},
+            client=client,
+        )
+        assert result["success"] is False
+        assert "before" in result["error"] and "after" in result["error"]
+        client.get_room_events.assert_not_awaited()
+
+    async def test_get_history_omits_the_cursor_when_none_was_asked_for(self):
+        """An unset cursor must reach the client as None, not as "": an empty
+        string is a value, and the server reads a cursor as a position."""
+        room = self._readable_room()
+        client = self._stub_client()
+        client.get_room = AsyncMock(return_value=_make_room_state(room))
+        client.get_room_events = AsyncMock(
+            return_value=self._timeline_page([_make_message(message_id="msg-1")])
+        )
+        await self._run(
+            _handle_chatto_get_history, {"room_id": "room-1"}, client=client
+        )
+        kwargs = client.get_room_events.await_args.kwargs
+        assert kwargs.get("before") is None
+        assert kwargs.get("after") is None
+
+    async def test_get_history_refuses_a_room_the_bot_has_not_joined(self):
+        """Room membership is required by every read RPC the plugin makes —
+        GetRoomEvents, GetMessage and BatchGetMessages all say so. Pre-empting
+        it with the /join remedy beats handing the agent a bare
+        PERMISSION_DENIED that names neither the room nor the fix."""
+        room = _make_room("room-9", "Lurk", RoomKind.CHANNEL)
+        client = self._stub_client()
+        client.get_room = AsyncMock(
+            return_value=_make_room_state(room, is_member=False)
+        )
+        client.get_room_events = AsyncMock()
+        result, _, _ = await self._run(
+            _handle_chatto_get_history, {"room_id": "room-9"}, client=client
+        )
+        assert result["success"] is False
+        assert "/join room-9" in result["error"]
+        client.get_room_events.assert_not_awaited()
+
+    async def test_get_message_refuses_a_room_the_bot_has_not_joined(self):
+        """The same server rule covers GetMessage/BatchGetMessages, and the
+        remedy is the same — one refusal, two tools, one room rule."""
+        room = _make_room("room-9", "Lurk", RoomKind.CHANNEL)
+        client = self._stub_client()
+        client.get_room = AsyncMock(
+            return_value=_make_room_state(room, is_member=False)
+        )
+        client.get_message = AsyncMock()
+        client.batch_get_messages = AsyncMock()
+        result, _, _ = await self._run(
+            _handle_chatto_get_message,
+            {"room_id": "room-9", "event_id": "msg-1"},
+            client=client,
+        )
+        assert result["success"] is False
+        assert "/join room-9" in result["error"]
+        client.get_message.assert_not_awaited()
+        client.batch_get_messages.assert_not_awaited()
+
     async def test_get_history_refuses_an_out_of_range_limit(self):
         result, _, _ = await self._run(
             _handle_chatto_get_history, {"room_id": "room-1", "limit": 101}