Prechádzať zdrojové kódy

Merge fix/mention-resolution-without-admin-calls: local mention resolution, no admin calls

Liv Klumpp 2 týždňov pred
rodič
commit
800df2c2a9
3 zmenil súbory, kde vykonal 67 pridanie a 38 odobranie
  1. 25 34
      adapter.py
  2. 15 0
      skills/chatto-platform-plugin/SKILL.md
  3. 27 4
      test_adapter.py

+ 25 - 34
adapter.py

@@ -429,8 +429,6 @@ class ChattoAdapter(BasePlatformAdapter):
 
         # Member directory cache: user_id -> user info dict
         self._user_cache: dict[str, User] = {}
-        # Handle -> does a user hold it. Cached both ways; see _mentions_someone_else.
-        self._known_handles: dict[str, bool] = {}
 
         # Chattolib client cache and lock for async access.
         self._chatto_client: ChattoClient | None = None
@@ -730,39 +728,25 @@ class ChattoAdapter(BasePlatformAdapter):
     async def _handle_belongs_to_a_user(self, handle: str) -> bool:
         """Whether ``handle`` is the login of a real Chatto user.
 
-        The API carries no mention entities — ``mention_confirmation_token`` is
-        reserved in the message descriptor — so an @-token is only a candidate
-        until the directory confirms it. Results are cached both ways under the
-        lowercased handle (the mention namespace is case-insensitive per
-        FDR-006), since the same handles recur and a miss is as reusable as a
-        hit. Only logins are looked up: matching another user's display name,
-        as the web frontend does against its room member list, has no directory
-        equivalent here.
+        Resolved strictly from local knowledge — the member cache the adapter
+        already keeps — never from the admin directory: ``admin_get_member``
+        needs admin permissions a bot account must not be assumed to hold,
+        and in the field its ``permission_denied`` failed every lookup, which
+        fail-open let the bot answer every @colleague message. The cache is
+        filled by the admission pipeline itself (room rosters are fetched for
+        every channel turn), presence patching and the edited-message path.
+
+        An unknown handle is "not confirmed", not "confirmed absent": Chatto
+        carries no mention entities (``mention_confirmation_token`` is
+        reserved in the message descriptor), so a @-token stays a candidate
+        we cannot verify — and staying silent on a false positive is worse
+        than answering one.
         """
-        cache_key = handle.lower()
-        known = self._known_handles.get(cache_key)
-        if known is not None:
-            return known
-        client = await self._get_chatto_client()
-        if client is None:
-            # Unresolved means "not confirmed", so the message goes through.
-            return False
-        try:
-            member = await client.admin_get_member(login=handle)
-        except Exception as exc:
-            logger.debug("Chatto: could not resolve handle @%s: %s", handle, exc)
-            return False
-        # admin_get_member returns pb_to_dict(GetMemberResponse):
-        # {"member": {"user": {...}, ...}, "roles": [...]} when found,
-        # or {} when not found.
-        if not member.get("member"):
-            self._known_handles[cache_key] = False
-            return False
-        from chattolib.types import User
-
-        user = User.parse(member["member"].get("user"))
-        self._known_handles[cache_key] = user is not None
-        return user is not None
+        lowered = handle.lower()
+        return any(
+            user is not None and (user.login or "").lower() == lowered
+            for user in self._user_cache.values()
+        )
 
     async def _mentions_someone_else(self, body: str) -> bool:
         """Whether the message @-mentions a person who is not this bot.
@@ -2014,6 +1998,13 @@ class ChattoAdapter(BasePlatformAdapter):
                 )
                 return None
 
+            # Handle resolution reads the member cache, which the roster
+            # projection fills. Fetch it now so the someone-else check below
+            # sees the room's members — the same projection is reused for
+            # this turn's channel_context further down, one fetch per room.
+            if policy == RoomPolicy.OPEN:
+                await self._roster_for_room(client, room_id=room_id)
+
             # In an open channel we see every message, including ones plainly
             # aimed at a named colleague. Answering those would be barging in,
             # so acknowledge that we read it and stay quiet. Checked after the

+ 15 - 0
skills/chatto-platform-plugin/SKILL.md

@@ -56,6 +56,21 @@ Looks up the user by login or ID and posts into the DM room.
 - **A half-delivered send says so.** The split, the thread routing and the stop-at-first-failure live in `_deliver_chunks`; `send()` turns its result into a `SendResult` and `_post_chunks` into a refusal carrying `partial: true` plus the `message_ids` that went out, so a retry repeats only what is missing. **Add a new send path by calling `_deliver_chunks`, not by writing a new loop** — two copies of the routing rules drift, and `send()` has to keep marking its own output seen (`on_posted`) so the realtime echo never comes back as input.
 - **Verify in BOTH contexts:** a gateway-session `tool_call` is not enough — also fire a `cronjob_manage action='run'` test when the tool will run from cron, because cron sessions exercise the config-fallback tier gateway sessions never hit. Debug order: `tool_result_contract` error → contract bug (async/return type); `AttributeError` on the client → wrong stub/service name; `permission_denied` → server-side limit.
 
+## Inbound gating — debugging "the bot answered a message it should have ignored"
+
+All admission gates live in `_admit_and_build` (adapter.py), shared by the posted and the edited path, and run in order: read-only-room drop, user auth, `REQUIRE_MENTION` drop of unaddressed messages, then the OPEN-room "someone else" check (`_mentions_someone_else` → 🫥 reaction, no dispatch). DMs skip every mention gate.
+
+Procedure when the bot answered a channel message it should not have:
+
+1. Read `~/.hermes/logs/agent.log` (DEBUG) around the message timestamp — the adapter logs every decision: `message_body: … room_kind:`, `could not resolve handle @x: <error>`, `acknowledging only`, `dispatching message to Hermes`. `~/.hermes/logs/gateway.log` (INFO) carries the summarized inbound line with room and session key.
+2. Establish the room's policy before blaming the gates: `gateway.platforms.chatto-platform.extra.require_mention_rooms` / `optional_mention_rooms` in `~/.hermes/config.yaml`, and the connect-time log line `currently joined in N room(s)`, which tags each room `[dm]` / `[on-mention]` / `[every-message]` / `[read-only]`.
+3. In an `[every-message]` (OPEN) room the only silencing gate is `_mentions_someone_else`, which confirms each @handle through `_handle_belongs_to_a_user` → `admin_get_member`. Treat fail-open resolution as a root-cause candidate: `permission_denied` resolves the handle to False ("not a mention"), so every foreign mention passes — and a bot without admin rights fails this systematically, not sporadically.
+
+Pitfalls:
+
+- **Fail-open gates degrade silently.** An unresolved handle reads as "not a mention", so the bot barges in on every @colleague message with no log line louder than DEBUG. Before defaulting a handle to False, prefer local knowledge the adapter already holds (`_user_cache`, filled by the room roster fetched for the very same dispatch) over a directory call — and cache the failure mode, otherwise each message re-pays the doomed API call.
+- **Mentions are extracted from body text** (`ChattoConstants.MENTION_RE`) outside code fences and inline code spans, matching the upstream web frontend — a handle quoted in code does not gate, and case-insensitivity applies throughout.
+
 ## Development workflow
 
 1. **Read the repo's `AGENTS.md` first** — it is the binding rulebook for all work on this repo: editing, linting/type-checking gates, shell-script checks, typed access, overriding, capabilities, README discipline. This skill does not repeat those rules.

+ 27 - 4
test_adapter.py

@@ -1400,12 +1400,13 @@ class TestForeignMention:
         adapter.handle_message.assert_called_once()
 
     async def test_broadcast_handles_are_shared_across_cases_in_the_cache(self):
-        """The directory cache keys on the lowercased handle: one lookup
-        serves '@bob' and '@BOB' alike."""
+        """The mention namespace is case-insensitive (FDR-006): '@BOB' and
+        '@bob' are the same person, and local resolution matches both."""
         adapter = self._adapter()
         await self._dispatch(adapter, "@BOB ping")
         await self._dispatch(adapter, "@bob again")
-        assert adapter._chatto_client.admin_get_member.await_count == 1
+        assert adapter._chatto_client.admin_get_member.await_count == 0
+        assert adapter.handle_message.await_count == 0
 
     async def test_talking_about_mentions_is_not_a_mention(self):
         """Verbatim from the field: the instruction to send a mention later must
@@ -1432,9 +1433,31 @@ class TestForeignMention:
         adapter = self._adapter()
         await self._dispatch(adapter, "@bob ping")
         await self._dispatch(adapter, "@bob again")
-        assert adapter._chatto_client.admin_get_member.await_count == 1
+        assert adapter._chatto_client.admin_get_member.await_count == 0
         assert adapter.handle_message.await_count == 0
 
+    async def test_bot_without_admin_rights_resolves_via_local_knowledge(self):
+        """From the field: a bot account without admin permissions gets
+        permission_denied from the admin directory for EVERY lookup, and the
+        fail-open fallthrough made the bot answer every @colleague message.
+        The roster projection of the very same dispatch already knows the
+        login, so a room member must be resolved locally without any
+        directory call at all."""
+        adapter = self._adapter()
+        adapter._chatto_client.admin_get_member = AsyncMock(
+            side_effect=PermissionError("permission_denied: permission denied")
+        )
+        adapter._chatto_client.list_room_members = AsyncMock(
+            return_value=(["user-1"], Page())
+        )
+        adapter._chatto_client.batch_get_room_members = AsyncMock(
+            return_value=[DirectoryMember(user=_make_user("user-1", "nickk"))]
+        )
+        await self._dispatch(adapter, "@nickk ein Test wegen Replies")
+        adapter.handle_message.assert_not_called()
+        adapter.add_reaction.assert_awaited_once()
+        adapter._chatto_client.admin_get_member.assert_not_awaited()
+
     async def test_an_email_address_is_not_a_mention(self):
         adapter = self._adapter()
         await self._dispatch(adapter, "schreib an bob@example.com")