Skip to content

Commit e81d337

Browse files
fix: address review — docs consistency, channel tests, random.choices
- DECISIONS.md: rename "Why Global Singleton on Chat" to "Why 3-Level Chat Resolver" with updated rationale reflecting the actual design - Add 7 ChannelImpl resolver tests (symmetric with ThreadImpl coverage): explicit chat=, activate(), beats global, concurrent isolation - Add 2 reviver channel tests (deserialize + bound to specific chat) - Comment random.choices in GChat _random_id() as cosmetic-only, not suitable for security purposes Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
1 parent bcf0e82 commit e81d337

3 files changed

Lines changed: 99 additions & 7 deletions

File tree

docs/DECISIONS.md

Lines changed: 10 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -50,19 +50,23 @@ The tradeoff is that we must maintain the JWKS fetching and JWT validation code
5050

5151
5. **Not classes**: The builders are functions that return TypedDicts, not class constructors. Using PascalCase for class constructors is standard Python. Using it for factory functions is a deliberate style choice for this domain.
5252

53-
## Why Global Singleton on Chat
53+
## Why 3-Level Chat Resolver
5454

55-
**Decision**: `Chat` registers itself as a global singleton via `set_chat_singleton(self)`. Thread and Channel deserialization uses `get_chat_singleton()` to resolve adapters.
55+
**Decision**: Thread and Channel deserialization resolves the active Chat instance through a 3-level chain: explicit `chat=` parameter → `ContextVar` → process-global fallback.
5656

5757
**Rationale**:
5858

59-
1. **Deserialization without context**: When a `ThreadImpl` is deserialized from JSON (e.g., from Redis state, from a queued entry, from modal context), it needs to resolve its adapter by name. Without a singleton, every deserialization call would need an explicit `chat` parameter threaded through the call stack.
59+
1. **Explicit is best**: `ThreadImpl.from_json(data, chat=chat)` is unambiguous and needs no ambient state. Preferred for library code and multi-tenant servers.
6060

61-
2. **Matches TS SDK**: The TS SDK uses `chat-singleton.ts` with the same pattern. Keeping the pattern identical reduces the cognitive load of syncing changes.
61+
2. **ContextVar for async isolation**: `chat.activate()` sets a task-local `ContextVar`. Concurrent async tasks can each have their own Chat without interference. This is the natural Python pattern for request-scoped state (same approach the Slack adapter uses for per-request auth context).
6262

63-
3. **Single Chat instance is the normal case**: In practice, applications have exactly one `Chat` instance. The singleton is registered during initialization and remains for the lifetime of the process.
63+
3. **Global fallback for simplicity**: `chat.register_singleton()` sets a process-global default. This is the TS SDK's pattern and works for the common single-Chat-per-process case. Existing code using `register_singleton()` is unchanged.
6464

65-
4. **Testing escape hatch**: `clear_chat_singleton()` and `has_chat_singleton()` are provided for test isolation. Each test can register its own singleton.
65+
4. **Resolution order**: `get_chat_singleton()` checks ContextVar first, then global, then raises `RuntimeError`. This means `activate()` always wins over the global, and explicit `chat=` always wins over both.
66+
67+
5. **Testing isolation**: Each test can use `with chat.activate():` or pass `chat=` explicitly instead of fighting over a single global. `clear_chat_singleton()` is still available for cleanup.
68+
69+
6. **Upstream sync cost is low**: The TS SDK uses a pure global. The Python resolver is a strict superset — `register_singleton()` and `get_chat_singleton()` still exist and behave identically for single-Chat usage. Upstream ports that touch deserialization paths work without modification.
6670

6771
## Why Zero Runtime Dependencies in Core
6872

src/chat_sdk/adapters/google_chat/adapter.py

Lines changed: 7 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -2709,7 +2709,13 @@ def __init__(
27092709

27102710

27112711
def _random_id() -> str:
2712-
"""Generate a short random ID similar to Math.random().toString(36).slice(2, 9)."""
2712+
"""Generate a short random ID for cosmetic use (card widget IDs).
2713+
2714+
NOT suitable for security-sensitive purposes (lock tokens, signatures, etc.)
2715+
— use ``secrets.token_hex()`` for those. This uses ``random.choices`` because
2716+
these IDs are only used as opaque Google Chat card widget suffixes and do not
2717+
need to be unpredictable.
2718+
"""
27132719
import random
27142720
import string
27152721

tests/test_chat_resolver.py

Lines changed: 82 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -15,6 +15,7 @@
1515
import pytest
1616

1717
from chat_sdk import Chat, MemoryStateAdapter
18+
from chat_sdk.channel import ChannelImpl
1819
from chat_sdk.testing import create_mock_adapter
1920
from chat_sdk.thread import (
2021
ThreadImpl,
@@ -41,6 +42,16 @@ def _thread_json(adapter_name: str = "slack") -> dict:
4142
}
4243

4344

45+
def _channel_json(adapter_name: str = "slack") -> dict:
46+
return {
47+
"_type": "chat:Channel",
48+
"id": f"{adapter_name}:C1",
49+
"channelVisibility": "public",
50+
"isDM": False,
51+
"adapterName": adapter_name,
52+
}
53+
54+
4455
class TestGlobalSingleton:
4556
"""Existing register_singleton pattern still works."""
4657

@@ -250,3 +261,74 @@ def test_reviver_passes_through_non_typed_values(self):
250261
assert reviver("key", "plain string") == "plain string"
251262
assert reviver("key", 42) == 42
252263
assert reviver("key", {"no_type": True}) == {"no_type": True}
264+
265+
def test_reviver_deserializes_channel(self):
266+
chat = _make_chat("slack")
267+
reviver = chat.reviver()
268+
data = _channel_json("slack")
269+
channel = reviver("key", data)
270+
assert isinstance(channel, ChannelImpl)
271+
assert channel.adapter.name == "slack"
272+
273+
def test_reviver_channel_bound_to_specific_chat(self):
274+
chat_a = _make_chat("a")
275+
chat_b = _make_chat("b")
276+
277+
reviver_a = chat_a.reviver()
278+
reviver_b = chat_b.reviver()
279+
280+
ch_a = reviver_a("k", _channel_json("a"))
281+
ch_b = reviver_b("k", _channel_json("b"))
282+
283+
assert ch_a.adapter.name == "a"
284+
assert ch_b.adapter.name == "b"
285+
286+
287+
class TestChannelImplResolver:
288+
"""Symmetric ChannelImpl coverage for the 3-level resolver."""
289+
290+
def setup_method(self):
291+
clear_chat_singleton()
292+
293+
def teardown_method(self):
294+
clear_chat_singleton()
295+
296+
def test_explicit_chat_parameter(self):
297+
chat = _make_chat("slack")
298+
data = _channel_json("slack")
299+
channel = ChannelImpl.from_json(data, chat=chat)
300+
assert channel.adapter.name == "slack"
301+
302+
def test_explicit_chat_beats_global(self):
303+
global_chat = _make_chat("global")
304+
explicit_chat = _make_chat("explicit")
305+
global_chat.register_singleton()
306+
307+
channel = ChannelImpl.from_json(_channel_json("explicit"), chat=explicit_chat)
308+
assert channel.adapter.name == "explicit"
309+
310+
def test_activate_resolves_channel(self):
311+
chat = _make_chat("slack")
312+
with chat.activate():
313+
channel = ChannelImpl.from_json(_channel_json("slack"))
314+
assert channel.adapter.name == "slack"
315+
316+
def test_explicit_chat_without_singleton(self):
317+
chat = _make_chat("solo")
318+
channel = ChannelImpl.from_json(_channel_json("solo"), chat=chat)
319+
assert channel.adapter.name == "solo"
320+
321+
async def test_concurrent_channel_isolation(self):
322+
chat_a = _make_chat("ca")
323+
chat_b = _make_chat("cb")
324+
results: dict[str, str] = {}
325+
326+
async def task(chat: Chat, name: str) -> None:
327+
with chat.activate():
328+
await asyncio.sleep(0.01)
329+
channel = ChannelImpl.from_json(_channel_json(name))
330+
results[name] = channel.adapter.name
331+
332+
await asyncio.gather(task(chat_a, "ca"), task(chat_b, "cb"))
333+
assert results["ca"] == "ca"
334+
assert results["cb"] == "cb"

0 commit comments

Comments
 (0)