Parcourir la source

Fix reaction wire format: Chatto accepts gemoji shortcodes only

Live logs showed the card seeds dying with `invalid_argument: unsupported
emoji` — the server's resolveEmojiInput takes gemoji shortcode names and
rejects every Unicode emoji, then echoes the name back in reaction events.
So both directions were wrong: the card paths (send_clarify,
_send_exec_approval_prompt) passed display emojis straight to
client.add_reaction, so no card ever had visible reaction anchors; and
incoming events delivered "white_check_mark"/"one" to a matcher comparing
Unicode glyphs, so every valid reaction answered "not a valid option".

- All add_reaction/remove_reaction callsites convert through
  _shortcode_for_reaction_emoji (one converter, replacing the
  half-mapped _emoji_to_shortcode helper whose coverage stopped at the
  lifecycle emojis — the reason processing markers worked while cards
  silently failed).
- Prompt-relevant shortcodes decode back to display emojis before
  matching; check variants arrive as "heavy_check_mark", so the alias
  fold runs on the decoded glyph.
- Tests pin the wire shape (seeds sent as shortcodes, events resolved
  from names) instead of feeding Unicode the server never delivers.
Paul Klumpp il y a 5 jours
Parent
commit
47d27b61b9
5 fichiers modifiés avec 221 ajouts et 41 suppressions
  1. 48 7
      AGENTS.md
  2. 9 0
      CONCEPTS.md
  3. 4 0
      README.md
  4. 63 26
      adapter.py
  5. 97 8
      test_adapter.py

+ 48 - 7
AGENTS.md

@@ -358,13 +358,19 @@ changes must keep them, or consciously renegotiate the docs.**
   session nobody is waiting in (the "Question-Loop" bug: the card worked, the
   answer never arrived). Emoji matching compares `_normalize_reaction_emoji`
   of the incoming reaction against keys the card builders also store
-  normalized: it strips variation selectors (U+FE0E/U+FE0F) and folds
-  check-mark variants (✔, ✔️, ☑️, ✓) onto ✅, because the card seeds ✅
-  (U+2705) while clients send whatever the emoji picker offers and may add or
-  strip a selector in transit. Keycap digits (1️⃣…9️⃣) keep their combining
-  keycap (U+20E3) — only the VS16 goes. Display forms (card text, seeded
-  reactions, the reply emoji in `_forward_reaction`) stay raw; only
-  *comparison* keys are normalized, on both sides at once. The prompt emoji
+  normalized: the incoming shortcode name is decoded to its display emoji
+  (`_SHORTCODE_TO_PROMPT_EMOJI`) first — Chatto's reaction API and realtime
+  events speak gemoji names, not Unicode (see *Research notes*) — then the
+  normalizer strips variation selectors (U+FE0E/U+FE0F) and folds
+  check-mark variants (✔, ✔️, ☑️, ✓, plus the shortcodes
+  `heavy_check_mark`/`ballot_box_with_check`) onto ✅, because the card
+  seeds ✅ (U+2705) while clients send whatever the emoji picker offers and
+  may add or strip a selector in transit. Keycap digits (1️⃣…9️⃣) keep their
+  combining keycap (U+20E3) — only the VS16 goes. Display forms (card text,
+  seeded reactions, the reply emoji in `_forward_reaction`) stay raw; only
+  *comparison* keys are normalized, on both sides at once. Outgoing
+  reactions are converted to shortcodes by `_shortcode_for_reaction_emoji` —
+  one converter for every `add_reaction` path. The prompt emoji
   sets live once as module constants (`_PROMPT_NUMBER_EMOJIS`,
   `_PROMPT_OTHER_EMOJI`, `_PROMPT_APPROVAL_EMOJIS` and their derived
   normalized key lists) — a fourth hand-copied list is the bug waiting to
@@ -920,6 +926,41 @@ the fix alone does not carry that knowledge:
   exec-approval card. Writing that down stops the next reader from
   re-testing the wrong prompt.
 
+### Reactions travel as gemoji shortcode names, not Unicode
+
+The report after the first fix ("still invalid") read as an emoji-variant
+problem — it was a wire-format problem. Chatto's AddReaction API accepts
+**only gemoji shortcode names** (`"one"`, `"white_check_mark"`); the server's
+`resolveEmojiInput` rejects everything else with `invalid_argument:
+unsupported emoji`, and the realtime `ReactionAddedEvent` echoes the name
+back. Consequences the adapter code must honour end to end:
+
+- **Outgoing seeds and lifecycle reactions are sent as shortcodes** — every
+  reaction goes through `_shortcode_for_reaction_emoji`. Raw Unicode is
+  rejected server-side; before the fix, the two card paths passed display
+  emojis straight to `client.add_reaction`, so **no card ever had visible
+  reaction anchors** and no lifecycle marker ever rendered.
+- **Incoming events carry the name.** Matching decodes
+  prompt-relevant names back to display emojis (`_SHORTCODE_TO_PROMPT_EMOJI`)
+  before `_normalize_reaction_emoji`, so the comparison domain stays the
+  seeded display form. The prompt-relevant subset lives in
+  `_PROMPT_SHORTCODES`; a client-side check variant arrives as
+  `"heavy_check_mark"` (not ✔️ — the alias fold runs on the decoded glyph).
+- **The half-map that hid this.** `EMOJI_TO_SHORTCODE` already existed and
+  covered the lifecycle emojis (👀/✅/❌/🚫/🫥), which is why processing
+  markers worked while every seeded card anchor silently failed — a
+  working neighbour made the broken path look sound. The keycap digits,
+  ✏️, 🌀 and ♾️ were missing from that map; the tests that passed did so
+  because they fed Unicode into the matcher, i.e. the mocks agreed on a
+  contract the wire does not have (the *Typed access* failure mode again,
+  one level up).
+- **Diagnosis source of truth:** the Chatto server repo
+  (`cli/internal/core/emoji.go` for `resolveEmojiInput`,
+  `cli/internal/core/emoji_gen.go` for the accepted names,
+  `reactions.go` for the event carrying `emojiName`). Anything sending a
+  bare emoji to this API is wrong by construction — verify against the
+  server, not against what our callsites happen to pass.
+
 So when a diagnosis required that kind of work, add a note here (or extend the
 relevant *Behavioural contract* entry) recording: what the wrong plausible
 reading was, which cross-repo sources had to be read together, and what makes

+ 9 - 0
CONCEPTS.md

@@ -408,3 +408,12 @@ 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.
+
+On top of that sits a translation layer the display forms never show: Chatto's
+reaction API takes gemoji shortcode names only, and its events echo those names
+back, so nothing the adapter sends or receives on the reaction path is the
+glyph the card displays. The shortcode conversion and the decode before
+matching exist because that wire format is invisible in the client — a user
+picks ✅ and never sees that "white_check_mark" travelled instead. The full
+diagnosis (server validation, the half-mapped converter that hid the gap, and
+why the tests missed it) is recorded in AGENTS.md's research notes.

+ 4 - 0
README.md

@@ -255,6 +255,10 @@ reactions (✔, ✔️, ☑️, ✓) are accepted where the card seeds ✅, and
 variation selector appended or removed by a client in transit does not
 change what a reaction means.
 
+The option emojis are seeded onto the card as Chatto reaction anchors —
+reacting to the card picks the option. (Chatto's reaction API accepts only
+shortcode names server-side; the adapter sends and matches accordingly, so
+this works regardless of which emoji form your client sends.)
 ### 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`).

+ 63 - 26
adapter.py

@@ -422,6 +422,8 @@ _CHECK_EMOJI_ALIASES: Final[frozenset[str]] = frozenset(
 def _normalize_reaction_emoji(emoji: str) -> str:
     """The comparison key for a reaction emoji.
 
+    Callers pass a decoded display emoji — shortcode names are translated by
+    ``_SHORTCODE_TO_PROMPT_EMOJI`` first (see ``_try_handle_interactive_reaction``).
     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
@@ -431,10 +433,52 @@ def _normalize_reaction_emoji(emoji: str) -> str:
     return "✅" if stripped in _CHECK_EMOJI_ALIASES else stripped
 
 
-def _log_emoji_codepoints(emoji: str) -> str:
-    """An emoji rendered as codepoints, for log lines that must distinguish
-    look-alikes — ✔️ (U+2714 FE0F) and ✅ (U+2705) differ only here."""
-    return " ".join(f"U+{ord(char):04X}" for char in emoji)
+# Shortcodes the Chatto server accepts for AddReaction — its API takes gemoji
+# names ("white_check_mark"), never Unicode, and rejects anything else with
+# `invalid_argument: unsupported emoji`. The realtime events echo the same
+# names back, so incoming reactions arrive as shortcodes too. Reactions sent
+# as Unicode were silently dropped server-side; the cards then showed no
+# reaction anchors and every matching reaction answered "not a valid option".
+_PROMPT_SHORTCODES: Final[dict[str, str]] = {
+    "1️⃣": "one",
+    "2️⃣": "two",
+    "3️⃣": "three",
+    "4️⃣": "four",
+    "5️⃣": "five",
+    "6️⃣": "six",
+    "7️⃣": "seven",
+    "8️⃣": "eight",
+    "9️⃣": "nine",
+    "✏️": "pencil2",
+    "✏": "pencil2",
+    "✅": "white_check_mark",
+    "🌀": "cyclone",
+    "♾️": "infinity",
+    "♾": "infinity",
+    "❌": "x",
+}
+
+
+def _shortcode_for_reaction_emoji(emoji: str) -> str:
+    """The shortcode name a Chatto reaction must be sent as.
+
+    Prompt emojis map explicitly; anything else falls through to the adapter's
+    general shortcode table. An unknown emoji is sent as-is — the server
+    rejects it with `unsupported emoji`, which the caller logs.
+    """
+    return _PROMPT_SHORTCODES.get(emoji) or ChattoConstants.EMOJI_TO_SHORTCODE.get(
+        emoji, emoji
+    )
+
+
+# Inverse of _PROMPT_SHORTCODES plus the check-mark aliases: the server's
+# realtime events carry shortcode names, and reaction matching compares the
+# display emojis the cards seed — so "white_check_mark" must read as ✅.
+_SHORTCODE_TO_PROMPT_EMOJI: Final[dict[str, str]] = {
+    name: emoji for emoji, name in _PROMPT_SHORTCODES.items()
+}
+_SHORTCODE_TO_PROMPT_EMOJI["heavy_check_mark"] = "✅"
+_SHORTCODE_TO_PROMPT_EMOJI["ballot_box_with_check"] = "✅"
 
 
 # Reaction glyphs for the interactive prompt cards. The display forms keep
@@ -2027,10 +2071,12 @@ class ChattoAdapter(BasePlatformAdapter):
             return False
         room_id = str(room_id)
         message_id = str(message_id)
-        # Keep the raw form for the log: normalizing first would hide exactly
-        # the codepoint a misbehaving client sent.
+        # Keep the raw form for the log: the server delivers shortcode names
+        # ("heavy_check_mark"), and a decoded-only log would hide exactly the
+        # name a client actually picked.
         raw_emoji = str(emoji)
-        emoji = _normalize_reaction_emoji(raw_emoji)
+        decoded = _SHORTCODE_TO_PROMPT_EMOJI.get(raw_emoji, raw_emoji)
+        emoji = _normalize_reaction_emoji(decoded)
         actor_id = str(actor_id)
 
         is_clarify = message_id in self._clarify_prompts
@@ -2038,10 +2084,10 @@ class ChattoAdapter(BasePlatformAdapter):
         if not is_clarify and not is_approval:
             return False
         logger.debug(
-            "Chatto: reaction on prompt card %s: %s (raw %s)",
+            "Chatto: reaction on prompt card %s: %s (raw %r)",
             message_id,
             emoji,
-            _log_emoji_codepoints(raw_emoji),
+            raw_emoji,
         )
 
         if is_clarify:
@@ -2699,6 +2745,10 @@ class ChattoAdapter(BasePlatformAdapter):
         if handler is None:
             return
 
+        # The server echoes the shortcode name back in the event ("thumbsup",
+        # "white_check_mark") — which is exactly the shape the mirrored Slack
+        # contract uses (Slack reaction events carry names too), so it is
+        # forwarded as-is rather than decoded.
         action = "removed" if removed else "added"
         try:
             await handler(
@@ -3407,7 +3457,7 @@ class ChattoAdapter(BasePlatformAdapter):
                 await client.add_reaction(
                     room_id=str(chat_id),
                     message_event_id=message_id,
-                    emoji=emoji,
+                    emoji=_shortcode_for_reaction_emoji(emoji),
                 )
             except Exception:
                 logger.debug("Chatto: seed reaction %s failed", emoji, exc_info=True)
@@ -3540,7 +3590,7 @@ class ChattoAdapter(BasePlatformAdapter):
                 await client.add_reaction(
                     room_id=str(prompt.chat_id),
                     message_event_id=message_id,
-                    emoji=emoji,
+                    emoji=_shortcode_for_reaction_emoji(emoji),
                 )
             except Exception:
                 logger.debug("Chatto: seed reaction %s failed", emoji, exc_info=True)
@@ -3782,25 +3832,12 @@ class ChattoAdapter(BasePlatformAdapter):
     # Reactions
     # ------------------------------------------------------------------ #
 
-    @staticmethod
-    def _emoji_to_shortcode(emoji: str) -> str:
-        """Convert a unicode emoji to a Chatto shortcode name.
-
-        If the emoji is already a shortcode (no unicode mapping found),
-        return it as-is.
-        """
-        shortcode = ChattoConstants.EMOJI_TO_SHORTCODE.get(emoji)
-        if shortcode:
-            return shortcode
-        # Already a shortcode like "thumbsup" — return as-is
-        return emoji
-
     async def add_reaction(self, room_id: str, message_id: str, emoji: str) -> bool:
         """Add a reaction to a message via MessageService/AddReaction.
 
         BasePlatformAdapter override
         """
-        shortcode = self._emoji_to_shortcode(emoji)
+        shortcode = _shortcode_for_reaction_emoji(emoji)
         client = await self._get_chatto_client()
         if client is None:
             logger.warning("Chatto: AddReaction — client unavailable")
@@ -3824,7 +3861,7 @@ class ChattoAdapter(BasePlatformAdapter):
 
         BasePlatformAdapter override
         """
-        shortcode = self._emoji_to_shortcode(emoji)
+        shortcode = _shortcode_for_reaction_emoji(emoji)
         client = await self._get_chatto_client()
         if client is None:
             logger.warning("Chatto: RemoveReaction — client unavailable")

+ 97 - 8
test_adapter.py

@@ -1383,6 +1383,28 @@ class TestInteractiveReactionRouting:
         assert "prompt-1" not in adapter._clarify_prompts
         assert "clarify-1" not in adapter._clarify_id_to_message
 
+    async def test_a_shortcode_reaction_event_resolves_the_clarify(self, adapter):
+        """The server echoes shortcodes, not Unicode: reacting on the web
+        frontend delivers "two" in the event, and that must resolve option 2."""
+        self._clarify(adapter)
+        resolve = MagicMock()
+        with patch("tools.clarify_gateway.resolve_gateway_clarify", resolve):
+            await adapter._handle_realtime_event(self._event("two"))
+
+        resolve.assert_called_once_with("clarify-1", "no")
+        assert "prompt-1" not in adapter._clarify_prompts
+
+    async def test_a_check_shortcode_reaction_resolves_the_approval(self, adapter):
+        """The user's bug, pinned twice over: the server delivers the check as
+        the shortcode name — "white_check_mark" from a web pick, or the client
+        variant ✔️ over the wire. Both must resolve "once"."""
+        self._approval(adapter, choices_map={"✅": "once", "❌": "deny"})
+        resolve = MagicMock()
+        with patch("tools.approval.resolve_gateway_approval", resolve):
+            await adapter._handle_realtime_event(self._event("white_check_mark"))
+        resolve.assert_called_once_with("session-1", "once")
+        assert "prompt-1" not in adapter._approval_prompts
+
     async def test_the_bots_own_reaction_does_not_resolve_its_prompt(self, adapter):
         """The self-event filter runs before routing, so an echo of the bot's
         own marker cannot answer the prompt it decorates."""
@@ -1452,15 +1474,37 @@ class TestInteractiveReactionRouting:
         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"."""
+        """The user's bug, pinned twice over: the wire carries gemoji names, so
+        the check arrives as "white_check_mark" (or the client variant alias
+        "heavy_check_mark" → ✔️) — both must resolve "once", never "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("heavy_check_mark"))
+        resolve.assert_called_once_with("session-1", "once")
+        assert "prompt-1" not in adapter._approval_prompts
+
         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("✔️"))
+            await adapter._handle_realtime_event(self._event("white_check_mark"))
+        resolve.assert_called_once_with("session-1", "once")
+        assert "prompt-1" not in adapter._approval_prompts
+
+    async def test_a_unicode_check_reaction_resolves_the_approval(self, adapter):
+        """Defensive: a client that sends the Unicode glyph instead of the
+        shortcode still resolves — the normalizer accepts both forms."""
+        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("✅\ufe0f"))
         resolve.assert_called_once_with("session-1", "once")
         assert "prompt-1" not in adapter._approval_prompts
 
@@ -1497,16 +1541,23 @@ class TestInteractiveReactionRouting:
         body = adapter._chatto_client.post_message.await_args.kwargs["body"]
         assert "✅ once" in body and "❌ deny" in body
 
-    async def test_a_routed_reaction_is_logged_with_its_codepoints(
+    async def test_a_routed_reaction_is_logged_with_its_wire_form(
         self, adapter, caplog
     ):
         """Diagnosing a rejected reaction from a live log requires seeing the
-        exact codepoints the client sent — ✔️ and ✅ differ only there."""
+        exact form the client sent — the shortcode name on the wire, plus the
+        emoji it decoded to."""
         self._approval(adapter)
         with caplog.at_level(logging.DEBUG, logger="adapter"):
-            await adapter._handle_realtime_event(self._event("✔️"))
+            await adapter._handle_realtime_event(self._event("heavy_check_mark"))
 
-        assert any("U+2714" in record.getMessage() for record in caplog.records)
+        message = next(
+            record.getMessage()
+            for record in caplog.records
+            if "reaction on prompt card" in record.getMessage()
+        )
+        assert "heavy_check_mark" in message
+        assert "✅" in message
 
 
 # -- Prompt card threading --
@@ -1529,6 +1580,7 @@ class TestPromptCardThreading:
             return_value=_make_message(body="card", message_id="card-1")
         )
         adapter._chatto_client.follow_thread = AsyncMock()
+        adapter._chatto_client.add_reaction = AsyncMock()
         return adapter
 
     async def test_clarify_card_threads_into_the_conversation(self, adapter):
@@ -1596,6 +1648,43 @@ class TestPromptCardThreading:
         assert "✅" in stored.choices_map
         assert stored.choices_map["✅"] == "once"
 
+    async def test_seed_reactions_are_sent_as_shortcodes(self, adapter):
+        """AddReaction accepts gemoji names only — a Unicode seed was rejected
+        server-side with `unsupported emoji`, leaving the card anchorless."""
+        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-3"
+        )
+        await adapter._send_exec_approval_prompt(prompt)
+
+        sent = [
+            call.kwargs["emoji"]
+            for call in adapter._chatto_client.add_reaction.await_args_list
+        ]
+        assert sent == ["white_check_mark", "x"]
+
+    async def test_clarify_seed_reactions_are_sent_as_shortcodes(self, adapter):
+        result = await adapter.send_clarify(
+            "room-1", "Which one?", ["yes", "no"], "clarify-1", "session-1"
+        )
+
+        assert result.success is True
+        sent = [
+            call.kwargs["emoji"]
+            for call in adapter._chatto_client.add_reaction.await_args_list
+        ]
+        assert sent == ["one", "two", "pencil2"]
+
     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