Browse Source

Housekeeping from the review: advice that works, diagnostics, and honest docs

Nine of the reviewer's minors, none of them behaviour the plugin promised.

- The unjoined-room refusal told a channel user to "send it a direct message",
  which cannot put the bot in a channel. Split by kind: an invite or the /join
  the bot answers over DM for channels, a DM with the bot account for DM rooms.
- The partial-delivery refusal lost the chunk position in the shared-loop
  extraction — "on chunk 2/3" is what makes retrying the remainder actionable.
  _ChunkDelivery's docstring promises it, so the code now keeps that promise.
- A configuration resolved without a token is no longer cached. Caching it
  pinned "token not configured" for the life of the process, so a call made
  later in a properly scoped context kept being served the degraded config.
- Dropped a new `# type: ignore[union-attr]` that AGENTS.md forbids, by building
  _ResolvedRoom in the comprehension instead of asserting after it.

Docs that said something the code does not do:

- README counted three tool boundaries and listed three; the membership refusal
  is a fourth, and CHATTO_LIST_ROOMS_SCHEMA now says is_member decides
  postability, so the model checks it rather than guessing.
- after-install.md claimed the bot "sees only rooms its account has joined",
  which the tool exists to contradict; plugin.yaml described CHATTO_ALLOWED_USERS
  as logins when it has always taken user IDs too.
- AGENTS.md told the reader that resolve() goes through coerce, which it does
  not — it goes through the wrappers, which reach the same _parse_* helpers.
  Reworded to the invariant (every path reaches them) rather than a mechanism
  that would have read as a rule to follow.
- Recorded the thread_sessions_per_user caveat where _seeded_threads is
  declared: the map is per thread root, that flag makes sessions per
  participant, so one participant's turn moves the cursor past the other's.
- SKILL.md still told a maintainer adding a send path to keep _post_chunks's
  contract — the line to point them at is _deliver_chunks now.

299 tests, all gates clean. Both new code behaviours mutation-checked.
Paul Klumpp 2 tuần trước cách đây
mục cha
commit
26400598e9
7 tập tin đã thay đổi với 106 bổ sung và 25 xóa
  1. 8 7
      AGENTS.md
  2. 2 1
      README.md
  3. 38 13
      adapter.py
  4. 3 2
      after-install.md
  5. 1 1
      plugin.yaml
  6. 1 1
      skills/chatto-platform-plugin/SKILL.md
  7. 53 0
      test_adapter.py

+ 8 - 7
AGENTS.md

@@ -491,13 +491,14 @@ configuration docs must be able to predict what happens when they set nothing.
 Secrets (`CHATTO_TOKEN`, `CHATTO_PASSWORD`) belong in `~/.hermes/.env`, never
 in `config.yaml` — docs and setup wizard rely on that split.
 
-A raw value from **anywhere** goes through `ConfigField.coerce`, never in
-as-is. `.env` lines are strings whatever the field means: a list field holding
-`"a,b"` turns `room_id in require_mention + optional_mention` into a substring
-test, and a bool field holding `"false"` is true. The parse lives once per kind
-in the `_parse_*` helpers; the `_get_env_or_extra_*` wrappers only decide where
-a value comes from. `resolve()` and `overlay_profile_secrets()` must both go
-through `coerce` — the overlay is the path a cron or CLI session takes, and it
+A raw value from **anywhere** is parsed by the field's kind before anything
+reads it. `.env` lines are strings whatever the field means: a list field
+holding `"a,b"` turns `room_id in require_mention + optional_mention` into a
+substring test, and a bool field holding `"false"` is true. The parse lives once
+per kind in the `_parse_*` helpers, and **every** path into a payload reaches
+them: `resolve()` via the `_get_env_or_extra_*` wrappers, and
+`overlay_profile_secrets()` via `ConfigField.coerce`. Both are required — the
+overlay is the path a cron or CLI session takes, and it
 is the one no env layer has already normalized.
 
 ### One config per serving profile, never one per process

+ 2 - 1
README.md

@@ -375,11 +375,12 @@ 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`. |
 
-Three boundaries are deliberate, and each tool reports them as an error instead of half-doing the job:
+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 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`.
+- **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.
 
 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.
 

+ 38 - 13
adapter.py

@@ -387,6 +387,13 @@ class ChattoAdapter(BasePlatformAdapter):
         # seen-ring; unlike the roster this is deliberately NOT discarded on
         # reconnect, since thread history is immutable.
         self._seeded_threads: dict[str, str | None] = {}
+        # Keyed by thread root, which assumes one session per thread. With
+        # thread_sessions_per_user turned on, each participant gets their own
+        # session while this map stays shared, so one participant's turn moves
+        # the cursor past the other's messages and that session is seeded once
+        # and never again. The flag is off by default and this is the price of
+        # switching it on; keying the map by session key would fix it and is
+        # noted next to _thread_context_for.
         # session_key -> message ID currently being processed there. Written
         # by on_processing_start, cleared by on_processing_complete; an edit
         # landing on the recorded ID is a mid-run correction.
@@ -3859,7 +3866,12 @@ def _chatto_config_for_tools() -> ChattoConfiguration | None:
         profile_secrets = {}
 
     resolved.overlay_profile_secrets(profile_secrets)
-    _resolved_chatto_configs[scope_key] = resolved
+    # Cache only a configuration that can actually send. Caching a tokenless one
+    # would pin this failure for the life of the process: the next call in a
+    # properly scoped context would be served the degraded config and keep
+    # reporting "token not configured" long after the scope came back.
+    if resolved.token.value:
+        _resolved_chatto_configs[scope_key] = resolved
     return resolved
 
 
@@ -3917,6 +3929,25 @@ def _room_is_writable(
     return _room_answers_in(cfg, room_id, room.kind)
 
 
+def _not_joined_refusal(room: Room) -> _ToolRefusal:
+    """The refusal for a room the bot can see but has not joined.
+
+    The two 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.
+    """
+    remedy = (
+        f"Invite the bot account in Chatto, or DM it `/join {room.id}`"
+        if room.kind == RoomKind.CHANNEL
+        else "Have someone start a DM with the bot account"
+    )
+    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."
+    )
+
+
 def _read_only_room_refusal(room_label: str) -> _ToolRefusal:
     """The refusal a read-only room earns, naming where to opt it in.
 
@@ -4026,7 +4057,8 @@ async def _post_chunks(
             else "Internal plugin error"
         )
         raise _refuse(
-            f"{cause}: {delivery.error}",
+            f"{cause} on chunk {len(delivery.message_ids) + 1}/"
+            f"{delivery.chunks_total}: {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),
@@ -4143,7 +4175,7 @@ async def _resolve_room(
         )
 
     matches = [
-        rws
+        _ResolvedRoom(rws.room, rws.viewer_state.is_member)
         for rws in await client.list_rooms()
         if rws.room is not None
         and (rws.room.name or "").casefold() == room_name.casefold()
@@ -4154,16 +4186,13 @@ async def _resolve_room(
         )
     if len(matches) > 1:
         candidates = ", ".join(
-            f"{rws.room.id} ('{rws.room.name}')"  # type: ignore[union-attr]
-            for rws in matches
+            f"{match.room.id} ('{match.room.name}')" for match in matches
         )
         raise _refuse(
             f"Room name '{room_name}' is ambiguous — {len(matches)} rooms carry it: "
             f"{candidates}. Pass the room_id instead."
         )
-    only = matches[0]
-    assert only.room is not None  # the filter above guarantees a room
-    return _ResolvedRoom(only.room, only.viewer_state.is_member)
+    return matches[0]
 
 
 async def _send_dm(args: dict) -> str:
@@ -4245,11 +4274,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 _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."
-            )
+            raise _not_joined_refusal(room)
 
         # A thread root has to belong to the room we post into, or the message
         # attaches to a thread somewhere else entirely. And it has to BE a root:

+ 3 - 2
after-install.md

@@ -69,8 +69,9 @@ can live in either file. Environment variables take precedence over
 
 5. Add the bot to a room
 
-   The bot sees only rooms its account has joined — membership lives on the
-   Chatto server, survives gateway restarts, and nothing is joined silently on
+   The bot can *see* more rooms than it has joined, but it answers and posts
+   only in rooms its account is a member of. Membership lives on the Chatto
+   server, survives gateway restarts, and nothing is joined silently on
    startup. Invite the bot account natively in Chatto, or DM it:
 
    ```

+ 1 - 1
plugin.yaml

@@ -35,7 +35,7 @@ optional_env:
     prompt: "Allow all users? (true/false)"
     password: false
   - name: CHATTO_ALLOWED_USERS
-    description: "Comma-separated Chatto logins allowed to talk to the agent (default: none — denies everyone unless CHATTO_ALLOW_ALL_USERS=true)"
+    description: "Comma-separated Chatto logins or user IDs allowed to talk to the agent (default: none — denies everyone unless CHATTO_ALLOW_ALL_USERS=true)"
     prompt: "Allowed users (comma-separated)"
     password: false
   - name: CHATTO_REQUIRE_MENTION_ROOMS

+ 1 - 1
skills/chatto-platform-plugin/SKILL.md

@@ -53,7 +53,7 @@ Looks up the user by login or ID and posts into the DM room.
 - **Early exits raise `_ToolRefusal`, they do not return.** The `async with _tool_client(cfg)` context manager and the `_refuse(...)` helper keep one result shape per failure; the `_handle_*` wrapper turns a refusal back into its JSON and catches `ChattoError`/`ChattoAuthError` around it. Never `except Exception: pass` — the AGENTS.md rule about naming each recovery applies here too.
 - **Config access: never read `os.environ` in handlers.** `.env` credentials like `CHATTO_TOKEN` are profile-scoped and NOT visible in cron fires' os.environ, and `hermes_validate_config` never runs in a fresh cron session. Resolve config through `_chatto_config_for_tools()` in adapter.py: (1) `_resolved_chatto_configs[_profile_scope_key()]` — the snapshot `hermes_validate_config` installed for *this* profile, only after it accepted the config; the map is keyed by `hermes_home_key()` because Hermes multiplexes profiles in one process, so a single module-level slot would hand the tools whichever profile validated last; (2) a fresh `load_gateway_config()` read for the `chatto-platform` platform entry, so `config.yaml`'s own `extra` block is honoured, re-filled from the installed secret scope (`agent.secret_scope.current_secret_scope()`) or, when none is installed, from `build_profile_secret_scope(get_hermes_home())`, which re-reads the profile's own `.env` and works in gateway, cron and CLI alike; (3) clean error.
 - **Never put a credential or a server URL in a tool schema.** The configured bot token must not be reachable from an argument: a `base_url` parameter would send it as `Authorization: Bearer …` to whatever host the caller names, and any message the bot reads is something the model could be talked into repeating. Take the client from `async with _tool_client(cfg) as client:` — it is the only sanctioned way to build one.
-- **A half-delivered send says so.** `_post_chunks` reports `partial: true` plus the `message_ids` that went out, so a retry repeats only what is missing. Keep that contract when adding a send path.
+- **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.
 
 ## Development workflow

+ 53 - 0
test_adapter.py

@@ -4153,6 +4153,24 @@ class TestPluginToolHandlers:
         assert "has joined" in result["error"]
         client.post_message.assert_not_awaited()
 
+    async def test_the_unjoined_refusal_offers_a_remedy_that_works_for_a_channel(self):
+        """A DM to the bot cannot get it into a channel — that advice would send
+        the user down a path that does not exist. The remedy for a channel is an
+        invite, or the /join the bot answers over DM."""
+        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 "/join room-1" in result["error"]
+
     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."""
@@ -4277,6 +4295,9 @@ class TestPluginToolHandlers:
         assert result["message_ids"] == ["msg-1"]
         assert result["chunks_sent"] == 1
         assert "rate limited" in result["error"]
+        # Which chunk failed is what makes "retry the rest" actionable; it used
+        # to say so and lost that in the shared-loop extraction.
+        assert "chunk 2/2" in result["error"]
 
     async def test_send_dm_reports_a_partial_delivery(self):
         client = self._stub_client()
@@ -4352,6 +4373,38 @@ class TestPluginToolHandlers:
         assert resolved.token.value == "yaml-token"
         assert resolved.base_url.value == "https://yaml.example"
 
+    async def test_a_tokenless_config_is_not_cached_for_the_process(self):
+        """Caching a configuration that could not send would pin the failure
+        for the life of the process: the next call, in a properly scoped
+        context, would be served the degraded config and keep reporting "token
+        not configured" long after the scope came back."""
+        import gateway.config as gw_config
+
+        empty = gw_config.PlatformConfig(enabled=True, extra={})
+        gateway_cfg = gw_config.GatewayConfig(
+            platforms={Platform(ChattoConstants.PLATFORM_NAME): empty}
+        )
+        with (
+            patch("adapter._resolved_chatto_configs", {}),
+            patch.object(gw_config, "load_gateway_config", return_value=gateway_cfg),
+            patch("agent.secret_scope.current_secret_scope", return_value=None),
+            patch(
+                "agent.secret_scope.build_profile_secret_scope",
+                return_value={},  # scope built, but the .env holds no token
+            ),
+        ):
+            degraded = _chatto_config_for_tools()
+            # Now the scope comes back, in the same process.
+            with patch(
+                "agent.secret_scope.build_profile_secret_scope",
+                return_value={"CHATTO_TOKEN": "cht_late"},
+            ):
+                recovered = _chatto_config_for_tools()
+
+        assert degraded is not None and degraded.token.value is None
+        assert recovered is not None
+        assert recovered.token.value == "cht_late"
+
     async def test_tool_config_is_scoped_to_the_serving_profile(
         self, tmp_path, monkeypatch
     ):