소스 검색

Thread prompt cards and normalize reaction emoji matching

Two user-reported failures, one shared root shape: both compare a key the
adapter builds against a key something else builds.

The Question-Loop: send_clarify and _send_exec_approval_prompt posted their
cards via post_message without a thread root, so the card landed at the room
root. An answer typed under the card is routed by session key — and Hermes'
build_session_key includes the thread ID — so the reply reached a fresh
session instead of the one waiting on the card, and every answer spawned the
next question. Both card senders now resolve the thread through
_resolve_outbound_thread, the same call send() uses, and follow it after
registration so replies group under the card.

The rejected check mark: the card seeds ✅ (U+2705) and matching compared
literal strings, but clients send whatever the emoji picker offers — ✔️
(U+2714 + VS16), plain ✓, ☑️ — and may add or strip variation selectors in
transit. Valid reactions were answered with "not a valid option". Both sides
of the comparison now go through _normalize_reaction_emoji (strips
U+FE0E/U+FE0F, folds check-mark variants onto ✅; keycaps keep their U+20E3),
and the prompt emoji sets are module constants so no fourth hand-copied list
reintroduces the bug.

The report read as a clarify-prompt bug, but ✅ is not a clarify option at
all — the failing case was the exec-approval card; see CONCEPTS.md.
Paul Klumpp 5 일 전
부모
커밋
8465278581
4개의 변경된 파일과 305개의 추가작업 그리고 21개의 파일을 삭제
  1. 18 0
      CONCEPTS.md
  2. 7 0
      README.md
  3. 109 21
      adapter.py
  4. 171 0
      test_adapter.py

+ 18 - 0
CONCEPTS.md

@@ -390,3 +390,21 @@ login→ID lookup on our side would have to scan member directories of rooms the
 search has not named yet, and `_find_user_by_login` deliberately only looks at
 DM rooms (a bot cannot read the server-wide directory). `author_id` is offered
 for the case where an ID is already in hand.
+
+## Prompt cards thread like any other send
+
+`send_clarify` and `_send_exec_approval_prompt` resolve the thread through the
+same `_resolve_outbound_thread` call as every other outbound path, because the
+card is not decoration — it is the message the session's answer is keyed to.
+A card posted at the room root keys to a session the gateway never routed the
+conversation into, so the answer's session key and the conversation's session
+key diverge silently: the reply works, the session it lands in is not the one
+waiting on the card, and the waiting turn stays blind forever.
+
+The normalization of reaction emojis exists for the same reason the threading
+does: the card is read by a client, not by the adapter. Matching compares the
+incoming emoji against the seeded one literally, and a ✔️ (U+2714) arriving for
+a seeded ✅ (U+2705) — or a variation selector added or stripped in transit —
+fails that comparison without either side being wrong. Normalization folds the
+comparison to the glyph the user means; the display forms on the card keep
+their selectors so rendering stays as the platform expects it.

+ 7 - 0
README.md

@@ -248,6 +248,13 @@ for free-text input; the exec-approval prompt offers ✅ once, 🌀 session,
 prompts are retired with a notice. When more than 9 choices are offered
 the adapter falls back to the default text-based prompt.
 
+Prompt cards are posted into the conversation they belong to (the same
+thread as the turn that asked), so a text answer typed under the card
+reaches the session that is waiting for it. Near-identical check-mark
+reactions (✔, ✔️, ☑️, ✓) are accepted where the card seeds ✅, and a
+variation selector appended or removed by a client in transit does not
+change what a reaction means.
+
 ### Room Member Roster
 
 Hermes cannot see who else is in a room: unaddressed channel messages are dropped by the mention gates long before they could teach him a name. So every turn in a channel carries the member roster alongside the message text — one entry per member as `@login (Display Name, presence)`, e.g. `@alice (Alice, online), @bob (Bob, offline)`. The bot's own account and deleted users are left out; rooms larger than 20 members end with `… and N more`. Presence is the Chatto status (`online`, `away`, `do not disturb`, `offline`).

+ 109 - 21
adapter.py

@@ -409,6 +409,58 @@ class _ApprovalPrompt:
 
 INTERACTIVE_PROMPT_TIMEOUT_SECONDS = 300
 
+# Check-mark variants that Chatto clients send for "the green check". The
+# prompts seed ✅ (U+2705), but emoji pickers offer near-identical glyphs with
+# a different codepoint — ✔️ adds a variation selector, ✓ is the plain text
+# check — and a client may also append or strip U+FE0F in transit. Matching
+# only the seeded form rejected the very reaction the prompt asks for.
+_CHECK_EMOJI_ALIASES: Final[frozenset[str]] = frozenset(
+    {"\u2705", "\u2714", "\u2713", "\u2611", "\u221a"}
+)
+
+
+def _normalize_reaction_emoji(emoji: str) -> str:
+    """The comparison key for a reaction emoji.
+
+    Strips variation selectors (U+FE0E/U+FE0F) so ✔️ and ✔ collapse, then
+    folds check-mark variants onto ✅. Keycap sequences (1️⃣ = digit + VS16 +
+    combining keycap) lose only the VS16 — digit and U+20E3 survive, and seed
+    and incoming reaction normalize to the same key.
+    """
+    stripped = emoji.replace("\ufe0e", "").replace("\ufe0f", "")
+    return "✅" if stripped in _CHECK_EMOJI_ALIASES else stripped
+
+
+# Reaction glyphs for the interactive prompt cards. The display forms keep
+# their variation selectors — they are what the card text and the seed
+# reactions show, and that is what should render as colour emoji. Reaction
+# *matching* never uses them directly: incoming emojis are normalized first
+# (_normalize_reaction_emoji), so every comparison happens against the
+# normalized *_KEY forms below.
+_PROMPT_NUMBER_EMOJIS: Final[tuple[str, ...]] = (
+    "1️⃣",
+    "2️⃣",
+    "3️⃣",
+    "4️⃣",
+    "5️⃣",
+    "6️⃣",
+    "7️⃣",
+    "8️⃣",
+    "9️⃣",
+)
+_PROMPT_OTHER_EMOJI: Final = "✏️"
+_PROMPT_APPROVAL_EMOJIS: Final[dict[str, str]] = {
+    "once": "✅",
+    "session": "🌀",
+    "always": "♾️",
+    "deny": "❌",
+}
+
+_NUMBER_EMOJI_KEYS: Final[tuple[str, ...]] = tuple(
+    _normalize_reaction_emoji(emoji) for emoji in _PROMPT_NUMBER_EMOJIS
+)
+_OTHER_EMOJI_KEY: Final = _normalize_reaction_emoji(_PROMPT_OTHER_EMOJI)
+
 
 _UNSEEDED: Final = object()
 """Sentinel: nothing is known yet about how far this thread's session reaches.
@@ -1969,7 +2021,7 @@ class ChattoAdapter(BasePlatformAdapter):
             return False
         room_id = str(room_id)
         message_id = str(message_id)
-        emoji = str(emoji)
+        emoji = _normalize_reaction_emoji(str(emoji))
         actor_id = str(actor_id)
 
         is_clarify = message_id in self._clarify_prompts
@@ -2023,11 +2075,9 @@ class ChattoAdapter(BasePlatformAdapter):
         if is_clarify:
             from tools import clarify_gateway  # type: ignore[import-not-found]
 
-            number_emojis = ["1️⃣", "2️⃣", "3️⃣", "4️⃣", "5️⃣", "6️⃣", "7️⃣", "8️⃣", "9️⃣"]
-            other_emoji = "✏️"
             choices = list(getattr(prompt, "choices", []) or [])
 
-            if emoji == other_emoji:
+            if emoji == _OTHER_EMOJI_KEY:
                 prompt.awaiting_text = True  # type: ignore[assignment]
                 try:
                     clarify_gateway.mark_awaiting_text(prompt.clarify_id)  # type: ignore[attr-defined]
@@ -2041,8 +2091,8 @@ class ChattoAdapter(BasePlatformAdapter):
                     logger.debug("Chatto: prompt edit failed", exc_info=True)
                 return True
 
-            if emoji in number_emojis:
-                idx = number_emojis.index(emoji)
+            if emoji in _NUMBER_EMOJI_KEYS:
+                idx = _NUMBER_EMOJI_KEYS.index(emoji)
                 if 0 <= idx < len(choices):
                     choice = choices[idx]
                     prompt.resolved = True
@@ -3263,9 +3313,9 @@ class ChattoAdapter(BasePlatformAdapter):
                 success=False, error="Chatto client not available", retryable=True
             )
 
-        number_emojis = ["1️⃣", "2️⃣", "3️⃣", "4️⃣", "5️⃣", "6️⃣", "7️⃣", "8️⃣", "9️⃣"]
+        number_emojis = _PROMPT_NUMBER_EMOJIS
         emojis = [number_emojis[i] for i in range(len(choices))]
-        other_emoji = "✏️"
+        other_emoji = _PROMPT_OTHER_EMOJI
 
         lines = [f"❓ {question}", ""]
         for i, choice in enumerate(choices):
@@ -3273,8 +3323,17 @@ class ChattoAdapter(BasePlatformAdapter):
         lines.append(f"{other_emoji} Other (type your answer)")
         content = "\n".join(lines)
 
+        # The card must carry the conversation's thread: an answer to it is
+        # routed by session key, and a card outside the thread keys to a
+        # different session than the one waiting for the answer.
+        thread_id = self._resolve_outbound_thread(chat_id, None, metadata)
+
         try:
-            msg = await client.post_message(room_id=str(chat_id), body=content)
+            msg = await client.post_message(
+                room_id=str(chat_id),
+                body=content,
+                thread_root_event_id=str(thread_id) if thread_id else "",
+            )
         except ChattoError as e:
             logger.warning("Chatto: send_clarify post failed: %s", e)
             return SendResult(success=False, error=str(e), retryable=True)
@@ -3307,6 +3366,17 @@ class ChattoAdapter(BasePlatformAdapter):
         self._clarify_prompts[message_id] = prompt
         self._clarify_id_to_message[clarify_id] = message_id
 
+        if thread_id:
+            try:
+                await client.follow_thread(chat_id, thread_id)
+            except Exception:
+                logger.debug(
+                    "Chatto: clarify follow_thread failed for %s/%s",
+                    chat_id,
+                    thread_id,
+                    exc_info=True,
+                )
+
         return SendResult(success=True, message_id=message_id)
 
     async def retire_clarify_card(self, clarify_id: str, notice: str) -> None:
@@ -3341,23 +3411,19 @@ class ChattoAdapter(BasePlatformAdapter):
 
         BasePlatformAdapter override
         """
-        emoji_map = {
-            "once": "✅",
-            "session": "🌀",
-            "always": "♾️",
-            "deny": "❌",
-        }
-        number_emojis = ["1️⃣", "2️⃣", "3️⃣", "4️⃣", "5️⃣", "6️⃣", "7️⃣", "8️⃣", "9️⃣"]
-
         choices = list(prompt.choices)
         choices_map: dict[str, str] = {}
         emojis: list[str] = []
         for i, choice in enumerate(choices):
-            emoji = emoji_map.get(choice)
+            emoji = _PROMPT_APPROVAL_EMOJIS.get(choice)
             if emoji is None:
-                emoji = number_emojis[i] if i < len(number_emojis) else "🔢"
+                emoji = (
+                    _PROMPT_NUMBER_EMOJIS[i] if i < len(_PROMPT_NUMBER_EMOJIS) else "🔢"
+                )
             emojis.append(emoji)
-            choices_map[emoji] = choice
+            # Keys in the normalized form: reaction matching looks the
+            # incoming emoji up here after _normalize_reaction_emoji.
+            choices_map[_normalize_reaction_emoji(emoji)] = choice
 
         client = await self._get_chatto_client()
         if client is None:
@@ -3389,7 +3455,18 @@ class ChattoAdapter(BasePlatformAdapter):
         content = "\n".join(lines)
 
         try:
-            msg = await client.post_message(room_id=str(prompt.chat_id), body=content)
+            # Same thread logic as send_clarify: the card must land in the
+            # conversation's thread, or a text answer to it keys to a session
+            # that is not the one waiting on the approval.
+            thread_id = self._resolve_outbound_thread(
+                str(prompt.chat_id), None, prompt.metadata
+            )
+
+            msg = await client.post_message(
+                room_id=str(prompt.chat_id),
+                body=content,
+                thread_root_event_id=str(thread_id) if thread_id else "",
+            )
         except ChattoError as e:
             logger.warning("Chatto: send_exec_approval post failed: %s", e)
             return SendResult(success=False, error=str(e), retryable=True)
@@ -3420,6 +3497,17 @@ class ChattoAdapter(BasePlatformAdapter):
         )
         self._approval_prompts[message_id] = approval_prompt
 
+        if thread_id:
+            try:
+                await client.follow_thread(str(prompt.chat_id), thread_id)
+            except Exception:
+                logger.debug(
+                    "Chatto: approval follow_thread failed for %s/%s",
+                    prompt.chat_id,
+                    thread_id,
+                    exc_info=True,
+                )
+
         return SendResult(success=True, message_id=message_id)
 
     async def edit_message(

+ 171 - 0
test_adapter.py

@@ -1359,6 +1359,19 @@ class TestInteractiveReactionRouting:
         adapter._clarify_id_to_message["clarify-1"] = "prompt-1"
         return prompt
 
+    def _approval(self, adapter, *, choices_map=None):
+        from adapter import _ApprovalPrompt
+
+        prompt = _ApprovalPrompt(
+            session_key="session-1",
+            chat_id="room-1",
+            message_id="prompt-1",
+            choices_map=choices_map or {"✅": "once"},
+            expires_at=float("inf"),
+        )
+        adapter._approval_prompts["prompt-1"] = prompt
+        return prompt
+
     async def test_a_numbered_reaction_resolves_the_clarify(self, adapter):
         self._clarify(adapter)
         resolve = MagicMock()
@@ -1407,6 +1420,164 @@ class TestInteractiveReactionRouting:
 
         handler.assert_called_once()
 
+    async def test_an_unrecognized_reaction_still_gets_the_feedback_path(self, adapter):
+        """Normalization folds aliases, not everything: an unknown emoji must
+        still take the invalid-emoji feedback path, not vanish."""
+        self._clarify(adapter)
+        await adapter._handle_realtime_event(self._event("🚀"))
+
+        assert "prompt-1" in adapter._clarify_prompts
+
+    async def test_the_other_emoji_reaction_is_recognized(self, adapter):
+        """✏️ routes to the awaiting-text path — the keycap-style normalization
+        must not have lost the Other button."""
+        self._clarify(adapter)
+        await adapter._handle_realtime_event(self._event("✏️"))
+
+        assert "prompt-1" in adapter._clarify_prompts
+        assert adapter._clarify_prompts["prompt-1"].awaiting_text is True
+
+    async def test_a_green_check_reaction_resolves_the_approval(self, adapter):
+        """The user's bug, pinned: the card seeds ✅ (U+2705) and the client
+        sends ✔️ (U+2714 + VS16) — the alias fold must accept the variant,
+        not answer "that reaction isn't a valid option"."""
+        self._approval(
+            adapter, choices_map={"✅": "once", "🌀": "session", "❌": "deny"}
+        )
+        resolve = MagicMock()
+        with patch("tools.approval.resolve_gateway_approval", resolve):
+            await adapter._handle_realtime_event(self._event("✔️"))
+        resolve.assert_called_once_with("session-1", "once")
+        assert "prompt-1" not in adapter._approval_prompts
+
+    async def test_a_selector_appended_check_resolves_the_approval(self, adapter):
+        """A client may also append the selector to the seeded form in
+        transit; the normalized comparison absorbs that too."""
+        self._approval(adapter)
+        resolve = MagicMock()
+        with patch("tools.approval.resolve_gateway_approval", resolve):
+            await adapter._handle_realtime_event(self._event("✅\ufe0f"))
+
+        resolve.assert_called_once_with("session-1", "once")
+
+    async def test_an_unknown_emoji_keeps_taking_the_feedback_path(self, adapter):
+        """The alias fold is a whitelist, not a free pass: 🚀 must not resolve
+        anything."""
+        self._approval(adapter)
+        resolve = MagicMock()
+        with patch("tools.approval.resolve_gateway_approval", resolve):
+            await adapter._handle_realtime_event(self._event("🚀"))
+
+        resolve.assert_not_called()
+        assert "prompt-1" in adapter._approval_prompts
+
+
+# -- Prompt card threading --
+
+
+class TestPromptCardThreading:
+    """Interactive prompt cards land in the conversation's thread.
+
+    An answer to a card is routed by session key — and that key includes the
+    thread ID. A card posted at the room root keys to a session that is not
+    the one waiting for the answer, so both card senders must resolve the
+    thread the same way ``send()`` does.
+    """
+
+    @pytest_asyncio.fixture
+    def adapter(self):
+        adapter = _make_adapter()
+        adapter._chatto_client = MagicMock()
+        adapter._chatto_client.post_message = AsyncMock(
+            return_value=_make_message(body="card", message_id="card-1")
+        )
+        adapter._chatto_client.follow_thread = AsyncMock()
+        return adapter
+
+    async def test_clarify_card_threads_into_the_conversation(self, adapter):
+        result = await adapter.send_clarify(
+            "room-1",
+            "Which one?",
+            ["yes", "no"],
+            "clarify-1",
+            "session-1",
+            metadata={"thread_id": "root-9"},
+        )
+
+        assert result.success is True
+        call_kwargs = adapter._chatto_client.post_message.call_args.kwargs
+        assert call_kwargs["thread_root_event_id"] == "root-9"
+        adapter._chatto_client.follow_thread.assert_awaited_once_with(
+            "room-1", "root-9"
+        )
+        assert "card-1" in adapter._clarify_prompts
+
+    async def test_approval_card_threads_into_the_conversation(self, adapter):
+        from gateway.platforms.base import ExecApprovalPrompt
+
+        prompt = ExecApprovalPrompt(
+            chat_id="room-1",
+            session_key="session-1",
+            text="Run this?",
+            actions=[("Approve", "once", "primary"), ("Deny", "deny", "danger")],
+            command="ls",
+            description="",
+            smart_denied=False,
+            metadata={"thread_id": "root-9"},
+        )
+
+        result = await adapter._send_exec_approval_prompt(prompt)
+
+        assert result.success is True
+        call_kwargs = adapter._chatto_client.post_message.call_args.kwargs
+        assert call_kwargs["thread_root_event_id"] == "root-9"
+        adapter._chatto_client.follow_thread.assert_awaited_once_with(
+            "room-1", "root-9"
+        )
+        assert "card-1" in adapter._approval_prompts
+
+    async def test_approval_card_reaction_matches_seeded_check(self, adapter):
+        """The seeded ✅ and the incoming ✔️ must meet in the normalized map:
+        the card registers its choices under normalized keys."""
+        from gateway.platforms.base import ExecApprovalPrompt
+
+        prompt = ExecApprovalPrompt(
+            chat_id="room-1",
+            session_key="session-1",
+            text="Run this?",
+            actions=[("Approve", "once", "primary"), ("Deny", "deny", "danger")],
+            command="ls",
+            description="",
+            smart_denied=False,
+        )
+        adapter._chatto_client.post_message.return_value = _make_message(
+            body="card", message_id="card-2"
+        )
+        await adapter._send_exec_approval_prompt(prompt)
+
+        stored = adapter._approval_prompts["card-2"]
+        assert "✅" in stored.choices_map
+        assert stored.choices_map["✅"] == "once"
+
+    async def test_cards_without_a_thread_stay_at_the_room_root(self, adapter):
+        """No metadata, no thread — a DM card (or a bare room) must not invent
+        one. The DM case is the strict one: _resolve_outbound_thread returns
+        None there even with a thread_id handed in."""
+        adapter._room_kinds["dm-1"] = RoomKind.DM
+        result = await adapter.send_clarify(
+            "dm-1",
+            "Which one?",
+            ["yes", "no"],
+            "clarify-dm",
+            "session-dm",
+            metadata={"thread_id": "root-9"},
+        )
+
+        assert result.success is True
+        call_kwargs = adapter._chatto_client.post_message.call_args.kwargs
+        assert call_kwargs["thread_root_event_id"] == ""
+        adapter._chatto_client.follow_thread.assert_not_awaited()
+
 
 # -- Realtime resume --