fix: address test review — eliminate mock theater, add edge cases

Critical fixes (tests that could pass with broken production code):
- Discord cache eviction test: was replicating eviction logic inline.
  Now calls _handle_forwarded_reaction which triggers the real code path.
- GitHub cache purge test: was replicating purge logic inline.
  Now calls _get_installation_token which runs the real purge.

Missing edge cases added to test_chat_resolver.py:
- from_json errors when no singleton and no chat= (lazy RuntimeError)
- from_json with mismatched adapter name (adapter resolves to None)
- activate() context manager resets ContextVar after exception

Other improvements:
- test_concurrent_tasks_isolated: use public API (get_adapter) instead
  of private _user_name attribute

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>

Author: patrick-chinchill

tests/test_chat_resolver.py

@@ -84,6 +84,40 @@ def test_error_when_no_singleton(self):
         with pytest.raises(RuntimeError, match="No Chat instance available"):
             get_chat_singleton()
 
+    def test_from_json_errors_when_no_singleton(self):
+        """from_json without chat= or singleton raises RuntimeError on adapter access."""
+        data = _thread_json("slack")
+        thread = ThreadImpl.from_json(data)
+        # Thread is created but adapter resolution fails lazily
+        with pytest.raises(RuntimeError, match="No Chat instance available"):
+            _ = thread.adapter
+
+    def test_from_json_with_mismatched_adapter_name(self):
+        """from_json with chat= but wrong adapter name returns None adapter."""
+        chat = _make_chat("slack")
+        data = _thread_json("nonexistent")
+        thread = ThreadImpl.from_json(data, chat=chat)
+        # Adapter was resolved via chat.get_adapter("nonexistent") → None
+        assert thread._adapter is None
+
+
+class TestActivateExceptionSafety:
+    """activate() context manager resets ContextVar even after exceptions."""
+
+    def setup_method(self):
+        clear_chat_singleton()
+
+    def teardown_method(self):
+        clear_chat_singleton()
+
+    def test_contextvar_reset_after_exception(self):
+        chat = _make_chat()
+        with pytest.raises(ValueError, match="test error"), chat.activate():
+            assert get_chat_singleton() is chat
+            raise ValueError("test error")
+        # ContextVar should be reset after exception
+        assert not has_chat_singleton()
+
 
 class TestContextVarActivation:
     """chat.activate() scopes resolution to the current context."""
@@ -181,22 +215,23 @@ def teardown_method(self):
     async def test_concurrent_tasks_isolated(self):
         chat_a = _make_chat("adapter_a")
         chat_b = _make_chat("adapter_b")
-        results: dict[str, str] = {}
+        results: dict[str, str | None] = {}
 
-        async def task(chat: Chat, name: str) -> None:
+        async def task(chat: Chat, adapter_name: str) -> None:
             with chat.activate():
-                # Yield to let the other task run
                 await asyncio.sleep(0.01)
                 resolved = get_chat_singleton()
-                results[name] = resolved._user_name
+                # Verify via public API: check which adapter is registered
+                a = resolved.get_adapter(adapter_name)
+                results[adapter_name] = a.name if a else None
 
         await asyncio.gather(
-            task(chat_a, "a"),
-            task(chat_b, "b"),
+            task(chat_a, "adapter_a"),
+            task(chat_b, "adapter_b"),
         )
 
-        assert results["a"] == "adapter_a-bot"
-        assert results["b"] == "adapter_b-bot"
+        assert results["adapter_a"] == "adapter_a"
+        assert results["adapter_b"] == "adapter_b"
 
     async def test_concurrent_from_json_isolated(self):
         chat_a = _make_chat("aa")

tests/test_production_fixes.py

@@ -514,9 +514,14 @@ def __init__(self, *, token=""):
 class TestDiscordThreadParentCacheEviction:
     """Discord thread parent cache should not grow unboundedly."""
 
-    def test_thread_parent_cache_eviction_on_overflow(self):
-        """When cache exceeds 1000 entries, expired entries are purged."""
-        from chat_sdk.adapters.discord.adapter import DiscordAdapter
+    async def test_thread_parent_cache_eviction_on_overflow(self):
+        """When cache exceeds 1000 entries during reaction handling, expired entries are purged."""
+        from unittest.mock import AsyncMock, MagicMock
+
+        from chat_sdk.adapters.discord.adapter import (
+            CHANNEL_TYPE_PUBLIC_THREAD,
+            DiscordAdapter,
+        )
         from chat_sdk.adapters.discord.types import DiscordAdapterConfig
 
         adapter = DiscordAdapter(
@@ -526,26 +531,36 @@ def test_thread_parent_cache_eviction_on_overflow(self):
                 application_id="test-app",
             )
         )
+        adapter._chat = MagicMock()
+        adapter._bot_user_id = "bot-user"
 
-        # Fill cache with 1001 entries, all expired
+        # Fill cache with 1001 expired entries
         past = time.time() - 100
         for i in range(1001):
             adapter._thread_parent_cache[f"channel-{i}"] = {
                 "parent_id": f"parent-{i}",
                 "expires_at": past,
             }
-
         assert len(adapter._thread_parent_cache) == 1001
 
-        # The eviction check in the adapter fires when len > 1000 during
-        # webhook handling. We can verify the cache structure directly:
-        # after simulated eviction logic, expired entries should be removed.
-        now = time.time()
-        expired = [k for k, v in adapter._thread_parent_cache.items() if v.get("expires_at", 0) <= now]
-        for k in expired:
-            del adapter._thread_parent_cache[k]
+        # Trigger the production code path: _handle_forwarded_reaction fetches
+        # channel info for threads, which triggers cache insertion + eviction
+        adapter._discord_fetch = AsyncMock(return_value={"parent_id": "real-parent"})
+        await adapter._handle_forwarded_reaction(
+            {
+                "channel_id": "new-thread-channel",
+                "message_id": "msg-1",
+                "user_id": "user-1",
+                "emoji": {"name": "👍"},
+                "channel_type": CHANNEL_TYPE_PUBLIC_THREAD,
+            },
+            added=True,
+        )
 
-        assert len(adapter._thread_parent_cache) == 0
+        # After eviction, all 1001 expired entries should be purged,
+        # leaving only the newly inserted one
+        assert len(adapter._thread_parent_cache) <= 2
+        assert "new-thread-channel" in adapter._thread_parent_cache
 
 
 class TestGChatUserInfoCacheEviction:
@@ -583,28 +598,27 @@ async def test_evicts_oldest_entries_past_max_size(self):
 class TestGitHubInstallationTokenCachePurge:
     """GitHub installation token cache should purge expired entries."""
 
-    def test_expired_entries_purged_on_access(self):
+    async def test_expired_entries_purged_on_access(self):
+        """Calling _get_installation_token purges expired entries from the cache."""
         from chat_sdk.adapters.github.adapter import GitHubAdapter
 
         adapter = GitHubAdapter({"webhook_secret": "test-secret", "token": "pat-123"})
 
-        # Manually populate the cache with expired entries
+        # Populate cache with expired entries
         past = time.time() - 100
         for i in range(10):
             adapter._installation_token_cache[i] = (f"token-{i}", past)
 
-        # Add one valid entry
+        # Add one valid entry (expires in 1 hour, with 60s buffer = still valid)
         future = time.time() + 3600
         adapter._installation_token_cache[999] = ("valid-token", future)
 
         assert len(adapter._installation_token_cache) == 11
 
-        # The purge logic runs at the start of _get_installation_token
-        # We simulate it directly:
-        now = time.time()
-        expired_ids = [iid for iid, (_, exp) in adapter._installation_token_cache.items() if now >= exp]
-        for iid in expired_ids:
-            del adapter._installation_token_cache[iid]
+        # Call the real method — it purges expired entries at the top,
+        # then finds the valid cached token for installation_id=999
+        token = await adapter._get_installation_token(999)
 
+        assert token == "valid-token"
         assert len(adapter._installation_token_cache) == 1
         assert 999 in adapter._installation_token_cache