address self-review findings on PR #48

- **Structural thread-ID validation**: reject `slack::`, `slack:::`,
  `slack::::` etc. via `any(seg for seg in remainder.split(":"))`
  BEFORE calling the adapter. Previously relied on
  `adapter.channel_id_from_thread_id()` returning exactly
  `"{adapter}:"` to catch the empty-remainder case, but different
  adapters return different things for malformed input (MockAdapter
  returns `"slack:"`, real Slack regex-matches and returns the full
  string unchanged, etc.). The structural check doesn't depend on
  adapter-specific behavior, so it's a single source of truth.
- **Expand `test_empty_remainder_raises`** to cover `slack:::` and
  `slack::::` variants.
- **Clean up `test_omits_history_when_adapter_does_not_persist`**:
  explicitly set `adapter.persist_message_history = None` instead of
  asserting on the mock's default. Prevents silent test regression
  if the mock default ever changes, and makes intent unambiguous.

Author: patrick-chinchill

src/chat_sdk/chat.py

@@ -1404,25 +1404,34 @@ def thread(
 
         Mirrors ``chat.thread(threadId)`` from the upstream TS SDK.
         """
-        # Validate thread ID shape: must be `{adapter}:{remainder}` with a
-        # non-empty adapter prefix AND non-empty remainder. Without the
-        # remainder check, `"slack:"` or `"slack::"` would construct a
-        # ThreadImpl with an empty channel ID that blows up on the first
-        # adapter call — surface the error here instead.
+        # Validate thread ID shape structurally, before calling the adapter.
+        # A valid ID is `{adapter}:{segment}[:{segment}...]` with:
+        #   - non-empty adapter prefix
+        #   - at least one non-empty segment after the prefix
+        # Rejects `"slack:"`, `"slack::"`, `"slack::::"` etc. at the SDK
+        # boundary so we don't rely on each adapter's
+        # `channel_id_from_thread_id` doing its own defense. (Different
+        # adapters return different things for malformed input; we need
+        # a single source of truth.)
         adapter_name, sep, remainder = thread_id.partition(":")
-        if not sep or not adapter_name or not remainder:
+        if not sep or not adapter_name:
             raise ChatError(f"Invalid thread ID: {thread_id}")
+        # Remainder is empty or only colons → no actual content.
+        if not remainder or not any(seg for seg in remainder.split(":")):
+            raise ChatError(f"Invalid thread ID: {thread_id}")
+
         adapter = self._adapters.get(adapter_name)
         if adapter is None:
             raise ChatError(f'Adapter "{adapter_name}" not found for thread ID "{thread_id}"')
 
-        # Defer to the adapter to derive channel_id — it knows the
-        # platform-specific encoding. Empty channel_id also means malformed.
+        # Defer to the adapter to derive channel_id as a secondary sanity
+        # check — some platform-specific patterns can pass the structural
+        # check but still be malformed (e.g. wrong number of segments).
         try:
             derived_channel_id = adapter.channel_id_from_thread_id(thread_id)
         except Exception as exc:
             raise ChatError(f"Invalid thread ID: {thread_id}") from exc
-        if not derived_channel_id or derived_channel_id == f"{adapter_name}:":
+        if not derived_channel_id:
             raise ChatError(f"Invalid thread ID: {thread_id}")
 
         stub_message = (

tests/test_chat_resolver.py

@@ -504,14 +504,18 @@ def test_reuses_parent_chat_state_and_history(self):
         assert thread._message_history is chat._message_history
 
     def test_omits_history_when_adapter_does_not_persist(self):
-        """Adapters with `persist_message_history=False`/None opt out of
-        the shared history cache. `Chat.thread()` respects that: the
-        Thread gets `None` for history rather than a shared cache the
-        adapter won't populate.
+        """Adapters with `persist_message_history` falsy opt out of the
+        shared history cache. `Chat.thread()` respects that: the Thread
+        gets `None` for history rather than a shared cache the adapter
+        won't populate.
+
+        Explicitly set `persist_message_history = None` rather than
+        relying on the mock's default — prevents silent test regression
+        if the mock default ever changes.
         """
         chat = _make_chat("slack")
         adapter = chat._adapters["slack"]
-        assert not adapter.persist_message_history  # default on mock is None
+        adapter.persist_message_history = None
 
         thread = chat.thread("slack:C123:1234567890.123456")
         assert thread._state_adapter is chat._state_adapter
@@ -525,17 +529,20 @@ def test_invalid_thread_id_raises(self):
             chat.thread("no-colon-here")
 
     def test_empty_remainder_raises(self):
-        """`slack:` or `slack::` would create a thread with empty channel
-        ID that blows up on the first adapter call — surface the error
-        at construction time instead.
+        """`slack:` or `slack::`, or any string where every segment after
+        the adapter prefix is empty, would create a thread with no real
+        content. Surface the error at construction time instead of
+        relying on adapter-specific `channel_id_from_thread_id` to
+        catch it (adapters differ in what they return for malformed
+        input — some return the input, some return a partial prefix —
+        so we validate structurally before dispatching to the adapter).
         """
         from chat_sdk.errors import ChatError
 
         chat = _make_chat("slack")
-        with pytest.raises(ChatError, match="Invalid thread ID"):
-            chat.thread("slack:")
-        with pytest.raises(ChatError, match="Invalid thread ID"):
-            chat.thread("slack::")
+        for bad in ("slack:", "slack::", "slack:::", "slack::::"):
+            with pytest.raises(ChatError, match="Invalid thread ID"):
+                chat.thread(bad)
 
     def test_unregistered_adapter_raises(self):
         from chat_sdk.errors import ChatError