From d0692a446d0ac5b05da096f851c377a2219f963f Mon Sep 17 00:00:00 2001 From: Claude Date: Sat, 30 May 2026 02:06:17 +0000 Subject: [PATCH 1/6] feat(slack): expose web_client property on SlackAdapter (#98) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Port the Slack adapter's direct WebClient access from upstream vercel/chat (commits 8366b8b / fdebde7 / 2f108bd, PRs #471/#476/#478). - Add ``SlackAdapter.web_client``: a synchronous ``slack_sdk.WebClient`` bound to the current request-context token (multi-workspace) or the configured default token (single-workspace). Token resolution uses the existing 3-level resolver via ``_get_token()``: ContextVar token > static ``bot_token`` config > ``AuthenticationError`` (no ``or`` fallbacks). - Add ``_get_web_client_for_token`` mirroring upstream's ``getClientForToken`` — one cached ``WebClient`` per distinct token. Kept separate from the async ``_client_cache`` (``AsyncWebClient``). ``slack_sdk`` import stays deferred (optional dependency, hazard #10). - Add deprecated ``client`` property alias delegating to ``web_client`` (one-release deprecation; emits ``DeprecationWarning``). Tests (tests/test_slack_web_client.py) mirror upstream's "webClient getter" block: single-tenant binding + per-token caching identity, multi-tenant ContextVar resolution under ``with_bot_token``, no-context and unresolved-async-resolver -> ``AuthenticationError``, and the deprecated alias returning the same object plus emitting the warning. https://claude.ai/code/session_01FyMxQn2BEAzmwKS1GZczKj --- src/chat_sdk/adapters/slack/adapter.py | 85 ++++++++++ tests/test_slack_web_client.py | 223 +++++++++++++++++++++++++ 2 files changed, 308 insertions(+) create mode 100644 tests/test_slack_web_client.py diff --git a/src/chat_sdk/adapters/slack/adapter.py b/src/chat_sdk/adapters/slack/adapter.py index 7cb2f1a..c40baf9 100644 --- a/src/chat_sdk/adapters/slack/adapter.py +++ b/src/chat_sdk/adapters/slack/adapter.py @@ -19,6 +19,7 @@ import os import re import time +import warnings from collections import OrderedDict from collections.abc import AsyncIterable, Awaitable, Callable from contextvars import ContextVar @@ -466,6 +467,16 @@ def __init__(self, config: SlackAdapterConfig | None = None) -> None: self._client_cache: OrderedDict[str, Any] = OrderedDict() self._client_cache_max = config.client_cache_max if config.client_cache_max is not None else 100 + # Cache of synchronous slack_sdk.WebClient instances keyed by bot + # token, backing the public ``web_client`` property (the direct port + # of upstream's ``getClientForToken``). Kept separate from + # ``_client_cache`` because that one holds async ``AsyncWebClient`` + # instances used by the adapter's own API calls; the two client types + # are not interchangeable. Mirrors upstream's plain (unbounded) Map — + # one entry per distinct token — since callers reach for this escape + # hatch rarely and tokens are low-cardinality. + self._web_client_cache: dict[str, Any] = {} + # Multi-workspace OAuth fields. # ``is not None`` (not truthiness) so an explicit empty-string user # config does not silently fall back to env (hazard #1). Empty env @@ -590,6 +601,61 @@ def current_client(self) -> Any: """ return self._get_client() + @property + def web_client(self) -> Any: + """Direct access to a synchronous ``slack_sdk.WebClient``. + + Bound to the bot token for the current request context + (multi-workspace) or the configured default token + (single-workspace). Use for any Slack Web API call not covered by + the adapter's high-level methods — e.g. + ``adapter.web_client.pins_add(...)`` or + ``adapter.web_client.usergroups_list(...)``. + + Resolution order (the standard 3-level resolver): + + 1. Token from the current request context (set during webhook + handling, or by :meth:`with_bot_token` / :meth:`with_bot_token_async`). + 2. The default bot token, when configured as a static string or + already-resolved value. + 3. Otherwise raise :class:`AuthenticationError`. + + Raises :class:`AuthenticationError` if neither is available — + typical causes are accessing ``web_client`` outside any + webhook / :meth:`with_bot_token` context in multi-workspace mode, + or having configured ``bot_token`` as an async resolver that has + not run yet. In the latter case await + :meth:`current_token_async` (or process the work inside the + webhook flow) so the resolver primes the token first. + + Return type is ``Any`` (rather than the concrete ``WebClient``) + because ``slack_sdk`` is an optional dependency — consumers who do + not install the ``slack`` extra should not pay an import cost. + + This is the direct port of upstream's ``adapter.webClient`` getter + (vercel/chat ``2f108bd``). Unlike :attr:`current_client` it returns + the *synchronous* ``WebClient`` (the analog of the single TS + ``WebClient``), so its methods are not awaitables. + """ + return self._get_web_client_for_token(self._get_token()) + + @property + def client(self) -> Any: + """Deprecated alias for :attr:`web_client`. + + .. deprecated:: + Use :attr:`web_client` instead. This alias mirrors upstream's + pre-rename ``adapter.client`` (vercel/chat ``8366b8b``) and is + kept for one release for backwards compatibility; it will be + removed in a future version. Emits :class:`DeprecationWarning`. + """ + warnings.warn( + "SlackAdapter.client is deprecated; use SlackAdapter.web_client instead.", + DeprecationWarning, + stacklevel=2, + ) + return self.web_client + # ------------------------------------------------------------------ # Token management # ------------------------------------------------------------------ @@ -743,6 +809,25 @@ def _invalidate_client(self, token: str) -> None: """Remove a cached client (e.g., on token revocation).""" self._client_cache.pop(token, None) + def _get_web_client_for_token(self, token: str) -> Any: + """Return a synchronous ``slack_sdk.WebClient`` for *token*, cached. + + Backs the public :attr:`web_client` property and is the direct port + of upstream's ``getClientForToken`` (vercel/chat ``2f108bd``): one + cached ``WebClient`` instance per distinct token. The import is + deferred so ``slack_sdk`` stays an optional dependency (hazard #10). + + Distinct from :meth:`_get_client`, which caches the *async* + ``AsyncWebClient`` used by the adapter's own API calls. + """ + client = self._web_client_cache.get(token) + if client is None: + from slack_sdk import WebClient + + client = WebClient(token=token) + self._web_client_cache[token] = client + return client + # ------------------------------------------------------------------ # Initialization # ------------------------------------------------------------------ diff --git a/tests/test_slack_web_client.py b/tests/test_slack_web_client.py new file mode 100644 index 0000000..8eea5a6 --- /dev/null +++ b/tests/test_slack_web_client.py @@ -0,0 +1,223 @@ +"""Tests for the public ``SlackAdapter.web_client`` property and ``client`` alias. + +Port of upstream vercel/chat's ``webClient getter`` test block +(``packages/adapter-slack/src/index.test.ts``), added by commits +``8366b8b`` / ``fdebde7`` / ``2f108bd`` (PRs #471 / #476 / #478): the Slack +adapter exposes a synchronous ``WebClient`` bound to the current +request-context token (multi-workspace) or the configured default token +(single-workspace), with a one-release deprecated ``client`` alias. + +``web_client`` resolves its token via the standard 3-level resolver +(ContextVar token > static default token > ``AuthenticationError``) and +caches one ``WebClient`` per distinct token. +""" + +from __future__ import annotations + +import sys +import warnings +from types import ModuleType + +import pytest + +# --------------------------------------------------------------------------- +# Stub slack_sdk so tests run without the real dependency installed. +# Stub BOTH the sync ``slack_sdk.WebClient`` (backing ``web_client``) and the +# async ``slack_sdk.web.async_client.AsyncWebClient`` (backing the adapter's +# own API calls) so the deferred imports inside the adapter resolve here. +# --------------------------------------------------------------------------- + + +class _FakeWebClient: + """Minimal stand-in for the synchronous ``slack_sdk.WebClient``.""" + + def __init__(self, *, token: str = "") -> None: + self.token = token + + +class _FakeAsyncWebClient: + """Minimal stand-in for ``slack_sdk.web.async_client.AsyncWebClient``.""" + + def __init__(self, *, token: str = "") -> None: + self.token = token + + +# Reuse any ``slack_sdk`` stub already registered by a sibling test module +# (e.g. ``test_slack_client_cache.py``) when collected in the same process — +# ``setdefault`` would otherwise no-op and leave a stub that lacks the sync +# ``WebClient`` symbol. Attach the symbols we need either way so the deferred +# imports inside the adapter resolve regardless of collection order. +_fake_slack_sdk = sys.modules.setdefault("slack_sdk", ModuleType("slack_sdk")) +_fake_slack_sdk_web = sys.modules.setdefault("slack_sdk.web", ModuleType("slack_sdk.web")) +_fake_slack_sdk_web_async = sys.modules.setdefault( + "slack_sdk.web.async_client", ModuleType("slack_sdk.web.async_client") +) + +if not hasattr(_fake_slack_sdk, "WebClient"): + _fake_slack_sdk.WebClient = _FakeWebClient # type: ignore[attr-defined] +if not hasattr(_fake_slack_sdk_web_async, "AsyncWebClient"): + _fake_slack_sdk_web_async.AsyncWebClient = _FakeAsyncWebClient # type: ignore[attr-defined] +_fake_slack_sdk_web.async_client = _fake_slack_sdk_web_async # type: ignore[attr-defined] +_fake_slack_sdk.web = _fake_slack_sdk_web # type: ignore[attr-defined] + +from chat_sdk.adapters.slack.adapter import SlackAdapter # noqa: E402, I001 +from chat_sdk.adapters.slack.types import SlackAdapterConfig # noqa: E402 +from chat_sdk.shared.errors import AuthenticationError # noqa: E402 + + +# --------------------------------------------------------------------------- +# Helpers +# --------------------------------------------------------------------------- + +_SECRET = "test-signing-secret" + + +def _single_workspace_adapter(bot_token: str = "xoxb-static-token") -> SlackAdapter: + """Single-workspace adapter with a static default bot token.""" + return SlackAdapter(SlackAdapterConfig(signing_secret=_SECRET, bot_token=bot_token)) + + +def _multi_workspace_adapter() -> SlackAdapter: + """Multi-workspace adapter (no default bot token; tokens are per-team).""" + return SlackAdapter( + SlackAdapterConfig( + signing_secret=_SECRET, + client_id="test-client-id", + client_secret="test-client-secret", + ) + ) + + +# --------------------------------------------------------------------------- +# Single-workspace +# --------------------------------------------------------------------------- + + +class TestWebClientSingleWorkspace: + def test_returns_client_bound_to_static_bot_token(self): + """``web_client`` returns a WebClient bound to the configured token.""" + adapter = _single_workspace_adapter("xoxb-static-token") + + web_client = adapter.web_client + + # Constructed via ``slack_sdk.WebClient`` (whichever class is + # registered in this process — real or sibling stub). + assert isinstance(web_client, _fake_slack_sdk.WebClient) + assert web_client.token == "xoxb-static-token" + + def test_returns_same_instance_per_token(self): + """Repeated access returns the exact same cached object (identity).""" + adapter = _single_workspace_adapter() + + assert adapter.web_client is adapter.web_client + + def test_caches_under_the_resolved_token(self): + """The cached client is keyed by the resolved token.""" + adapter = _single_workspace_adapter("xoxb-cache-key") + + client = adapter.web_client + + assert adapter._web_client_cache["xoxb-cache-key"] is client + + +# --------------------------------------------------------------------------- +# Deprecated ``client`` alias +# --------------------------------------------------------------------------- + + +class TestClientAlias: + def test_alias_returns_same_object_as_web_client(self): + """The deprecated ``client`` alias returns the same instance.""" + adapter = _single_workspace_adapter() + + with warnings.catch_warnings(): + warnings.simplefilter("ignore", DeprecationWarning) + assert adapter.client is adapter.web_client + + def test_alias_emits_deprecation_warning(self): + """Accessing ``client`` warns; ``web_client`` does not.""" + adapter = _single_workspace_adapter() + + with warnings.catch_warnings(record=True) as caught: + warnings.simplefilter("always") + _ = adapter.client + + deprecations = [w for w in caught if issubclass(w.category, DeprecationWarning)] + assert len(deprecations) == 1 + assert "web_client" in str(deprecations[0].message) + + def test_web_client_does_not_warn(self): + """The non-deprecated ``web_client`` property emits no warning.""" + adapter = _single_workspace_adapter() + + with warnings.catch_warnings(record=True) as caught: + warnings.simplefilter("always") + _ = adapter.web_client + + deprecations = [w for w in caught if issubclass(w.category, DeprecationWarning)] + assert deprecations == [] + + +# --------------------------------------------------------------------------- +# Multi-workspace (request-context token resolution) +# --------------------------------------------------------------------------- + + +class TestWebClientMultiWorkspace: + def test_uses_request_context_token_under_with_bot_token(self): + """Inside ``with_bot_token`` the context token wins.""" + adapter = _multi_workspace_adapter() + + observed: dict[str, str] = {} + + def capture() -> None: + observed["token"] = adapter.web_client.token + + adapter.with_bot_token("xoxb-context-token", capture) + + assert observed["token"] == "xoxb-context-token" + + def test_context_token_overrides_static_default(self): + """A context token takes precedence over the configured default.""" + adapter = _single_workspace_adapter("xoxb-default") + + observed: dict[str, str] = {} + + def capture() -> None: + observed["token"] = adapter.web_client.token + + adapter.with_bot_token("xoxb-override", capture) + + assert observed["token"] == "xoxb-override" + # Outside the context, resolution falls back to the static default. + assert adapter.web_client.token == "xoxb-default" + + def test_raises_without_context_in_multi_workspace_mode(self): + """No context + no default token raises on both accessors.""" + adapter = _multi_workspace_adapter() + + with pytest.raises(AuthenticationError): + _ = adapter.web_client + + with warnings.catch_warnings(): + warnings.simplefilter("ignore", DeprecationWarning) + with pytest.raises(AuthenticationError): + _ = adapter.client + + +# --------------------------------------------------------------------------- +# Async resolver: not-yet-resolved default token raises (sync property) +# --------------------------------------------------------------------------- + + +class TestWebClientAsyncResolver: + def test_unresolved_async_resolver_raises(self): + """A callable ``bot_token`` that has not run yet cannot resolve sync.""" + + async def resolver() -> str: + return "xoxb-async-resolved" + + adapter = SlackAdapter(SlackAdapterConfig(signing_secret=_SECRET, bot_token=resolver)) + + with pytest.raises(AuthenticationError): + _ = adapter.web_client From a7f5b53957bc3f032defd838f53fb0e32a617a40 Mon Sep 17 00:00:00 2001 From: Claude Date: Sat, 30 May 2026 02:10:18 +0000 Subject: [PATCH 2/6] fix(slack): evict web_client cache on invalidation + clarify caching test (review) Two review follow-ups on the web_client port: - Gemini: `_invalidate_client` cleared only the async `_client_cache`, leaving stale/revoked synchronous `WebClient` instances in `_web_client_cache` on token revocation / auth-error eviction. Pop the token from both caches. New `test_invalidate_client_clears_web_client_cache` is load-bearing. - github-code-quality: `assert adapter.web_client is adapter.web_client` tripped "comparison of identical values". The test is a genuine caching check (the property is invoked twice), but binding each access to a name makes that intent explicit and silences the false-positive. https://claude.ai/code/session_01FyMxQn2BEAzmwKS1GZczKj --- src/chat_sdk/adapters/slack/adapter.py | 1 + tests/test_slack_web_client.py | 23 ++++++++++++++++++++++- 2 files changed, 23 insertions(+), 1 deletion(-) diff --git a/src/chat_sdk/adapters/slack/adapter.py b/src/chat_sdk/adapters/slack/adapter.py index c40baf9..d119902 100644 --- a/src/chat_sdk/adapters/slack/adapter.py +++ b/src/chat_sdk/adapters/slack/adapter.py @@ -808,6 +808,7 @@ def _get_client(self, token: str | None = None) -> Any: def _invalidate_client(self, token: str) -> None: """Remove a cached client (e.g., on token revocation).""" self._client_cache.pop(token, None) + self._web_client_cache.pop(token, None) def _get_web_client_for_token(self, token: str) -> Any: """Return a synchronous ``slack_sdk.WebClient`` for *token*, cached. diff --git a/tests/test_slack_web_client.py b/tests/test_slack_web_client.py index 8eea5a6..537deba 100644 --- a/tests/test_slack_web_client.py +++ b/tests/test_slack_web_client.py @@ -109,7 +109,13 @@ def test_returns_same_instance_per_token(self): """Repeated access returns the exact same cached object (identity).""" adapter = _single_workspace_adapter() - assert adapter.web_client is adapter.web_client + # Bind each property access to a name so the identity check reads as a + # caching assertion (two calls → same object), not a tautological + # ``x is x`` self-comparison. + first = adapter.web_client + second = adapter.web_client + + assert first is second def test_caches_under_the_resolved_token(self): """The cached client is keyed by the resolved token.""" @@ -119,6 +125,21 @@ def test_caches_under_the_resolved_token(self): assert adapter._web_client_cache["xoxb-cache-key"] is client + def test_invalidate_client_clears_web_client_cache(self): + """``_invalidate_client`` evicts the sync WebClient too (token revocation). + + Load-bearing: without the ``_web_client_cache.pop`` in + ``_invalidate_client`` a revoked token's stale ``WebClient`` would + survive eviction and this assertion fails. + """ + adapter = _single_workspace_adapter("xoxb-revoke") + client = adapter.web_client + assert adapter._web_client_cache["xoxb-revoke"] is client + + adapter._invalidate_client("xoxb-revoke") + + assert "xoxb-revoke" not in adapter._web_client_cache + # --------------------------------------------------------------------------- # Deprecated ``client`` alias From dcb798a97f47a59d4c33b4747afd4b46fde87504 Mon Sep 17 00:00:00 2001 From: Claude Date: Sat, 30 May 2026 03:33:17 +0000 Subject: [PATCH 3/6] fix(slack): invoke sync token resolvers in web_client / _get_token (codex review) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The sync ``_get_token`` path only handled the static-string and primed cache cases — a sync ``bot_token`` callable (used e.g. for secret rotation or lazy load from a sync source) raised ``AuthenticationError`` from ``web_client`` outside any webhook / ContextVar scope until an async path had primed the cache. Proactive sends from single-workspace apps using a sync resolver therefore failed. Detect the sync-resolver case via ``inspect.iscoroutinefunction``, invoke the callable, validate the result, and prime ``_default_bot_token_cache`` with the same semantics the async ``_resolve_default_token`` path uses. Async resolvers still raise from the sync property (cannot be awaited). Defensive check for sync callables that *return* a coroutine (rare but real: ``lambda: some_async_fn()``) — refuse to cache the coroutine. The two existing tests that asserted the previous deficient behavior (sync resolver raising before resolution) are updated to assert the new correct behavior; the cache-refresh regression test switches to an async resolver so its sanity precondition still holds. --- src/chat_sdk/adapters/slack/adapter.py | 39 ++++++- .../test_slack_dynamic_token_and_verifier.py | 45 ++++++-- tests/test_slack_web_client.py | 106 ++++++++++++++++++ 3 files changed, 175 insertions(+), 15 deletions(-) diff --git a/src/chat_sdk/adapters/slack/adapter.py b/src/chat_sdk/adapters/slack/adapter.py index d119902..b1a3151 100644 --- a/src/chat_sdk/adapters/slack/adapter.py +++ b/src/chat_sdk/adapters/slack/adapter.py @@ -689,13 +689,42 @@ def _get_token(self) -> str: if self._default_bot_token_cache is not None: return self._default_bot_token_cache if self._default_bot_token_provider is not None: - # Resolver-based default token configured but never resolved. This - # is a programming error: the async entry path should have awaited - # ``_resolve_default_token()`` before reaching here. + provider = self._default_bot_token_provider + # Sync callable resolver: invoke it directly and prime the + # process-wide cache so subsequent sync access stays fast. Mirrors + # the cache-update semantics in ``_resolve_default_token`` (the + # async path). Async resolvers cannot be awaited from a sync + # context, so those still raise below — call ``current_token_async`` + # or enter via ``handle_webhook`` to prime the cache first. + if not inspect.iscoroutinefunction(provider): + resolved = provider() + # Defensive: a "sync" callable may still *return* a coroutine + # (e.g. ``lambda: some_async_fn()``) and ``iscoroutinefunction`` + # would not catch that. Caching a coroutine in + # ``_default_bot_token_cache`` would be a latent bug, so detect + # and raise instead. + if inspect.isawaitable(resolved): + raise AuthenticationError( + "slack", + "Bot token resolver returned an awaitable in a sync " + "context. Use the async API (handle_webhook / " + "current_token_async) so the resolver can be awaited.", + ) + if not isinstance(resolved, str) or not resolved: + raise AuthenticationError( + "slack", + "Bot token resolver returned an empty or non-string value.", + ) + self._default_bot_token_cache = resolved + return resolved + # Async resolver-based default token configured but never resolved. + # Cannot be awaited from a sync context — the async entry path must + # have awaited ``_resolve_default_token()`` before reaching here. raise AuthenticationError( "slack", - "Bot token resolver has not been invoked yet. Use the async API " - "(handle_webhook / current_token_async) so the resolver runs first.", + "Async bot token resolver has not been invoked yet. Use the " + "async API (handle_webhook / current_token_async) so the " + "resolver runs first.", ) raise AuthenticationError( "slack", diff --git a/tests/test_slack_dynamic_token_and_verifier.py b/tests/test_slack_dynamic_token_and_verifier.py index 0fbf91e..b762f79 100644 --- a/tests/test_slack_dynamic_token_and_verifier.py +++ b/tests/test_slack_dynamic_token_and_verifier.py @@ -181,18 +181,37 @@ def resolver() -> str: assert await adapter.current_token_async() == "xoxb-token-3" assert i[0] == 3 - def test_sync_current_token_with_resolver_before_resolution_raises(self): - """Sync ``current_token`` access before the resolver has run raises a clear error.""" + def test_sync_current_token_with_sync_resolver_invokes_resolver(self): + """Sync ``current_token`` invokes a sync resolver directly (Codex P2 fix). + + Previously the sync path raised ``AuthenticationError`` for sync + resolvers too — single-workspace apps using a sync resolver for + secret rotation could not read ``current_token`` / ``web_client`` + outside a webhook context until an async path had primed the cache. + ``_get_token`` now invokes the sync resolver directly and primes + ``_default_bot_token_cache``; the async-resolver-in-sync-context + case still raises (see + ``test_sync_current_token_with_async_resolver_raises`` below). + """ def resolver() -> str: return "xoxb-resolved" adapter = SlackAdapter(SlackAdapterConfig(signing_secret="s", bot_token=resolver)) - # Tightened: error message must mention ``current_token_async`` so - # callers know the right async accessor to use, not just that "the - # resolver hasn't run". Substring check on "current_token_async" - # is escaped via ``re.escape`` so the underscore isn't treated as a - # regex token (it isn't, but be explicit). + assert adapter.current_token == "xoxb-resolved" + # And the resolved token must be cached in the same slot the async + # path writes, so subsequent sync reads don't re-invoke. + assert adapter._default_bot_token_cache == "xoxb-resolved" + + def test_sync_current_token_with_async_resolver_raises(self): + """Async resolvers still cannot be awaited from the sync property.""" + + async def resolver() -> str: + return "xoxb-async-resolved" + + adapter = SlackAdapter(SlackAdapterConfig(signing_secret="s", bot_token=resolver)) + # Error message must point at the async entry point so callers know + # the right accessor to use. with pytest.raises(AuthenticationError, match=r"current_token_async"): _ = adapter.current_token @@ -316,6 +335,12 @@ async def test_resolver_refreshes_sync_token_cache(self): refresh the process-wide ``_default_bot_token_cache`` so the sync path returns the freshly resolved value. + Uses an *async* resolver so the sanity precondition (sync access + before any resolution raises) still holds — sync resolvers now + resolve directly on the sync path (Codex P2 fix), so the regression + scenario this test guards is specifically the async path priming the + sync cache. + What to fix if this fails: in ``_resolve_default_token`` (``adapters/slack/adapter.py``), after the ``isinstance(token, str)`` / non-empty validation and before @@ -323,13 +348,13 @@ async def test_resolver_refreshes_sync_token_cache(self): ``self._default_bot_token_cache = token``. """ - def resolver() -> str: + async def resolver() -> str: return "xoxb-resolved-token" adapter = SlackAdapter(SlackAdapterConfig(signing_secret="s", bot_token=resolver)) - # Sanity: before the resolver runs, sync ``current_token`` must raise - # (matches ``test_sync_current_token_with_resolver_before_resolution_raises``). + # Sanity: before the async resolver runs, sync ``current_token`` must + # raise — async resolvers cannot be awaited from the sync property. with pytest.raises(AuthenticationError, match=r"current_token_async"): _ = adapter.current_token diff --git a/tests/test_slack_web_client.py b/tests/test_slack_web_client.py index 537deba..f43ec50 100644 --- a/tests/test_slack_web_client.py +++ b/tests/test_slack_web_client.py @@ -242,3 +242,109 @@ async def resolver() -> str: with pytest.raises(AuthenticationError): _ = adapter.web_client + + +# --------------------------------------------------------------------------- +# Sync resolver: invoked directly from ``web_client`` (Codex P2 fix) +# --------------------------------------------------------------------------- + + +class TestWebClientSyncResolver: + """Cover the sync-callable ``bot_token`` branch in ``_get_token``. + + Before the fix, ``_get_token`` only handled the static-string and primed + cache cases — sync callables (used e.g. for lazy secret-manager loads or + rotation) raised ``AuthenticationError`` from ``web_client`` outside any + webhook / ContextVar scope, so proactive sends were impossible until an + async path had primed the cache. The new sync-callable branch invokes + the resolver and primes ``_default_bot_token_cache`` in the same way + ``_resolve_default_token`` does on the async path. + """ + + def test_sync_callable_resolves_and_caches(self): + """A sync ``bot_token`` callable is invoked on first access and cached. + + Load-bearing: with the fix reverted the first access raises + ``AuthenticationError`` instead of returning the resolved token. + """ + calls = {"n": 0} + + def resolver() -> str: + calls["n"] += 1 + return "xoxb-sync-resolved" + + adapter = SlackAdapter(SlackAdapterConfig(signing_secret=_SECRET, bot_token=resolver)) + + first = adapter.web_client + assert first.token == "xoxb-sync-resolved" + assert calls["n"] == 1 + + # Second access must hit the cache and not re-invoke the resolver — + # matches the cache semantics of the async ``_resolve_default_token`` + # path (which writes ``_default_bot_token_cache``). + second = adapter.web_client + assert second is first + assert calls["n"] == 1 + assert adapter._default_bot_token_cache == "xoxb-sync-resolved" + + def test_async_callable_in_sync_context_raises(self): + """``async def`` resolvers cannot be awaited from the sync property.""" + + async def resolver() -> str: + return "xoxb-async-resolved" + + adapter = SlackAdapter(SlackAdapterConfig(signing_secret=_SECRET, bot_token=resolver)) + + with pytest.raises(AuthenticationError) as excinfo: + _ = adapter.web_client + + # Not brittle about exact wording — just confirm the message points at + # the sync/async resolver mismatch rather than the generic + # "no bot token" path. + message = str(excinfo.value).lower() + assert "resolver" in message + assert "async" in message + + def test_sync_callable_returning_coroutine_raises(self): + """Defensive: a sync callable that returns a coroutine must not be cached.""" + + async def _coro() -> str: + return "xoxb-would-be-resolved" + + produced: list = [] + + def resolver(): + # ``iscoroutinefunction`` returns False for this — the awaitable + # only appears at call time. Caching the coroutine would be a + # latent bug; the defensive check must raise instead. + c = _coro() + produced.append(c) + return c + + adapter = SlackAdapter(SlackAdapterConfig(signing_secret=_SECRET, bot_token=resolver)) + + try: + with pytest.raises(AuthenticationError) as excinfo: + _ = adapter.web_client + + message = str(excinfo.value).lower() + assert "awaitable" in message or "async" in message + # And the cache must not have been poisoned with the coroutine object. + assert adapter._default_bot_token_cache is None + finally: + # Close the coroutine we created but intentionally did not await, + # so the test does not leave an un-awaited-coroutine warning behind. + for c in produced: + c.close() + + def test_sync_callable_returning_empty_string_raises(self): + """An empty/invalid resolver result raises rather than caching it.""" + + def resolver() -> str: + return "" + + adapter = SlackAdapter(SlackAdapterConfig(signing_secret=_SECRET, bot_token=resolver)) + + with pytest.raises(AuthenticationError): + _ = adapter.web_client + assert adapter._default_bot_token_cache is None From 41c8f3168aca427faff50aa6f018d6e4ea5d5b02 Mon Sep 17 00:00:00 2001 From: Claude Date: Sat, 30 May 2026 03:55:33 +0000 Subject: [PATCH 4/6] fix(slack): close orphan coroutine before raising awaitable-resolver error (audit) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The defensive `inspect.isawaitable(resolved)` branch in `_get_token`'s sync-callable handler raised AuthenticationError but never closed the coroutine the resolver returned. Callers saw a noisy `RuntimeWarning: coroutine was never awaited` on every triggering call. Close the awaitable via its `close()` method (Coroutine protocol) before raising. The existing regression test `test_sync_callable_returning_coroutine_raises` is strengthened to capture warnings and assert none of "never awaited" kind leaked — confirmed load-bearing under `pytest -W error::RuntimeWarning`. https://claude.ai/code/session_01FyMxQn2BEAzmwKS1GZczKj --- src/chat_sdk/adapters/slack/adapter.py | 7 +++++ tests/test_slack_web_client.py | 42 +++++++++++++++----------- 2 files changed, 32 insertions(+), 17 deletions(-) diff --git a/src/chat_sdk/adapters/slack/adapter.py b/src/chat_sdk/adapters/slack/adapter.py index b1a3151..94984e6 100644 --- a/src/chat_sdk/adapters/slack/adapter.py +++ b/src/chat_sdk/adapters/slack/adapter.py @@ -704,6 +704,13 @@ def _get_token(self) -> str: # ``_default_bot_token_cache`` would be a latent bug, so detect # and raise instead. if inspect.isawaitable(resolved): + # Close to suppress "coroutine was never awaited" + # RuntimeWarning before raising. ``isawaitable`` matches + # both coroutines and awaitable objects; both implement + # ``close`` via their ``__await__`` / Coroutine protocol. + close = getattr(resolved, "close", None) + if callable(close): + close() raise AuthenticationError( "slack", "Bot token resolver returned an awaitable in a sync " diff --git a/tests/test_slack_web_client.py b/tests/test_slack_web_client.py index f43ec50..83b2297 100644 --- a/tests/test_slack_web_client.py +++ b/tests/test_slack_web_client.py @@ -306,36 +306,44 @@ async def resolver() -> str: assert "async" in message def test_sync_callable_returning_coroutine_raises(self): - """Defensive: a sync callable that returns a coroutine must not be cached.""" + """Defensive: a sync callable that returns a coroutine must not be cached. + + The production code is responsible for closing the orphaned coroutine + before raising — otherwise callers see a noisy ``coroutine was never + awaited`` RuntimeWarning. We pin that behavior by capturing warnings + and asserting none were emitted. + """ + import warnings async def _coro() -> str: return "xoxb-would-be-resolved" - produced: list = [] - def resolver(): # ``iscoroutinefunction`` returns False for this — the awaitable # only appears at call time. Caching the coroutine would be a - # latent bug; the defensive check must raise instead. - c = _coro() - produced.append(c) - return c + # latent bug; the defensive check must raise AND close the + # coroutine to silence the warning. + return _coro() adapter = SlackAdapter(SlackAdapterConfig(signing_secret=_SECRET, bot_token=resolver)) - try: + with warnings.catch_warnings(record=True) as captured: + warnings.simplefilter("always") with pytest.raises(AuthenticationError) as excinfo: _ = adapter.web_client - message = str(excinfo.value).lower() - assert "awaitable" in message or "async" in message - # And the cache must not have been poisoned with the coroutine object. - assert adapter._default_bot_token_cache is None - finally: - # Close the coroutine we created but intentionally did not await, - # so the test does not leave an un-awaited-coroutine warning behind. - for c in produced: - c.close() + message = str(excinfo.value).lower() + assert "awaitable" in message or "async" in message + # Cache must not have been poisoned with the coroutine object. + assert adapter._default_bot_token_cache is None + # No "coroutine was never awaited" RuntimeWarning leaked — production + # code closed the coroutine before raising. + unawaited = [ + str(w.message) + for w in captured + if issubclass(w.category, RuntimeWarning) and "never awaited" in str(w.message) + ] + assert unawaited == [], f"unexpected un-awaited coroutine warnings: {unawaited}" def test_sync_callable_returning_empty_string_raises(self): """An empty/invalid resolver result raises rather than caching it.""" From e8a7dae72bb1f5be98194c6b45e4be8aef8a1eed Mon Sep 17 00:00:00 2001 From: Claude Date: Sat, 30 May 2026 17:14:04 +0000 Subject: [PATCH 5/6] fix(slack): honor bot_token rotation contract in sync _get_token + invalidation MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Addresses two P2 review findings on PR #127: **Codex P2 — sync resolver rotation broken** The previous sync-callable branch in ``_get_token`` cached the first resolved value in ``_default_bot_token_cache`` and the cache-first early-return prevented re-invocation, freezing rotating resolvers (e.g., secret-manager-backed). The contract on ``SlackAdapterConfig.bot_token`` says callable resolvers are "called on each use to support rotation." Track ``_is_dynamic_bot_token`` at construction time. In ``_get_token``, sync dynamic resolvers now invoke fresh on every call and never write the process-wide cache. Static-string configs keep their cache fast path (nothing to rotate). Async resolvers still require a webhook / ``current_token_async`` entry to be awaited. The previously-added test ``test_sync_current_token_with_sync_resolver_invokes_resolver`` asserted the cache was primed — flipped to assert the inverse, with a cross-reference to the dedicated rotation pin in ``test_sync_callable_invoked_fresh_each_access``. **CodeRabbit P2 — _invalidate_client retained revoked tokens** ``_invalidate_client(token)`` evicted the WebClient and AsyncWebClient caches but left ``_default_bot_token_cache`` / ``_resolved_default_token`` holding the revoked value, so the next ``_get_token`` returned the same token and the adapter just rebuilt clients around it. Now clears the resolved-token caches for dynamic-resolver configs so the next access re-invokes the resolver. Guarded on ``_is_dynamic_bot_token`` so static-string configs retain their cache (no refresh path — clearing would only make subsequent sync access raise with no way to recover). Tests: rewrote the caching test to assert rotation (resolver invoked fresh on every access, cache stays None); added invalidation tests covering dynamic-resolver clearing, ContextVar clearing, static-string no-op, and token-mismatch no-op. Full suite green (4067 passed) under ``-W error::RuntimeWarning``. https://claude.ai/code/session_01FyMxQn2BEAzmwKS1GZczKj --- src/chat_sdk/adapters/slack/adapter.py | 115 +++++++++-------- .../test_slack_dynamic_token_and_verifier.py | 16 ++- tests/test_slack_web_client.py | 119 +++++++++++++++--- 3 files changed, 178 insertions(+), 72 deletions(-) diff --git a/src/chat_sdk/adapters/slack/adapter.py b/src/chat_sdk/adapters/slack/adapter.py index 94984e6..921841e 100644 --- a/src/chat_sdk/adapters/slack/adapter.py +++ b/src/chat_sdk/adapters/slack/adapter.py @@ -396,6 +396,12 @@ def __init__(self, config: SlackAdapterConfig | None = None) -> None: # on each subsequent webhook entry. Static-string configs prime this at # construction time so sync access works before any webhook fires. self._default_bot_token_cache: str | None = bot_token_config if isinstance(bot_token_config, str) else None + # True when the user passed a callable ``bot_token`` (sync or async). + # The config contract says callable resolvers are invoked on each use + # to support rotation, so sync access goes through a fresh-invoke + # branch instead of reading ``_default_bot_token_cache``. Static + # strings have nothing to rotate and stay on the cached fast path. + self._is_dynamic_bot_token: bool = callable(bot_token_config) # ------------------------------------------------------------------ # Socket mode wiring (PR #86). Resolved AFTER the bot-token resolver @@ -668,17 +674,19 @@ def _get_token(self) -> str: 1. Multi-workspace request context (``_request_context``) 2. Per-request resolved default token (``_resolved_default_token``) — primed by ``handle_webhook`` after invoking the resolver - 3. Static default token cache (set by the constructor for - string-typed ``bot_token`` configs) - 4. Raises :class:`AuthenticationError` - - Synchronous: the token resolver is *not* invoked here. Webhook entry - paths call :meth:`_resolve_default_token` first so the per-request - cache is primed; static-string ``bot_token`` configs prime the - process-wide cache at construction time so this is a no-op for the - common case. Use :meth:`_resolve_token_async` from new async call - sites that need to invoke a resolver outside webhook flow (e.g. - cron jobs). + 3. Sync dynamic resolver — invoked **fresh on every call** to honor + the rotation contract in :attr:`SlackAdapterConfig.bot_token` + ("called on each use to support rotation") + 4. Static default token cache (set by the constructor for + string-typed ``bot_token`` configs, or by the async path after + ``_resolve_default_token`` runs) + 5. Raises :class:`AuthenticationError` + + Async resolvers cannot be awaited from a sync context — sync access + outside a webhook scope falls back to the process-wide cache, or + raises if the async path has not run yet. Use + :meth:`current_token_async` or enter via :meth:`handle_webhook` so + the resolver runs first. """ ctx = self._request_context.get() if ctx and ctx.token: @@ -686,47 +694,40 @@ def _get_token(self) -> str: per_request = self._resolved_default_token.get() if per_request is not None: return per_request + # Sync dynamic resolver: invoke fresh every call to honor rotation. + # Static strings (no rotation possible) and async resolvers (which + # need a webhook entry to be awaited) fall through to the cache. + provider = self._default_bot_token_provider + if self._is_dynamic_bot_token and provider is not None and not inspect.iscoroutinefunction(provider): + resolved = provider() + # Defensive: a "sync" callable may still *return* a coroutine + # (e.g. ``lambda: some_async_fn()``) and ``iscoroutinefunction`` + # would not catch that. Refuse to use such a value in a sync + # context — and close the awaitable to suppress the + # ``coroutine was never awaited`` RuntimeWarning before raising. + if inspect.isawaitable(resolved): + close = getattr(resolved, "close", None) + if callable(close): + close() + raise AuthenticationError( + "slack", + "Bot token resolver returned an awaitable in a sync " + "context. Use the async API (handle_webhook / " + "current_token_async) so the resolver can be awaited.", + ) + if not isinstance(resolved, str) or not resolved: + raise AuthenticationError( + "slack", + "Bot token resolver returned an empty or non-string value.", + ) + # Intentionally do NOT write ``_default_bot_token_cache``: caching + # would break the rotation contract for the next sync access. + return resolved if self._default_bot_token_cache is not None: return self._default_bot_token_cache - if self._default_bot_token_provider is not None: - provider = self._default_bot_token_provider - # Sync callable resolver: invoke it directly and prime the - # process-wide cache so subsequent sync access stays fast. Mirrors - # the cache-update semantics in ``_resolve_default_token`` (the - # async path). Async resolvers cannot be awaited from a sync - # context, so those still raise below — call ``current_token_async`` - # or enter via ``handle_webhook`` to prime the cache first. - if not inspect.iscoroutinefunction(provider): - resolved = provider() - # Defensive: a "sync" callable may still *return* a coroutine - # (e.g. ``lambda: some_async_fn()``) and ``iscoroutinefunction`` - # would not catch that. Caching a coroutine in - # ``_default_bot_token_cache`` would be a latent bug, so detect - # and raise instead. - if inspect.isawaitable(resolved): - # Close to suppress "coroutine was never awaited" - # RuntimeWarning before raising. ``isawaitable`` matches - # both coroutines and awaitable objects; both implement - # ``close`` via their ``__await__`` / Coroutine protocol. - close = getattr(resolved, "close", None) - if callable(close): - close() - raise AuthenticationError( - "slack", - "Bot token resolver returned an awaitable in a sync " - "context. Use the async API (handle_webhook / " - "current_token_async) so the resolver can be awaited.", - ) - if not isinstance(resolved, str) or not resolved: - raise AuthenticationError( - "slack", - "Bot token resolver returned an empty or non-string value.", - ) - self._default_bot_token_cache = resolved - return resolved - # Async resolver-based default token configured but never resolved. - # Cannot be awaited from a sync context — the async entry path must - # have awaited ``_resolve_default_token()`` before reaching here. + if provider is not None: + # Async resolver configured but never awaited. ``handle_webhook`` + # or ``current_token_async`` must run first to prime the cache. raise AuthenticationError( "slack", "Async bot token resolver has not been invoked yet. Use the " @@ -842,9 +843,21 @@ def _get_client(self, token: str | None = None) -> Any: return client def _invalidate_client(self, token: str) -> None: - """Remove a cached client (e.g., on token revocation).""" + """Remove a cached client (e.g., on token revocation). + + For dynamic-resolver configs also clears the resolved-token caches + so the next access re-invokes the resolver instead of serving the + revoked token. Static-string configs intentionally retain their + cache: there is no refresh path, so clearing would just make every + subsequent sync access raise. + """ self._client_cache.pop(token, None) self._web_client_cache.pop(token, None) + if self._is_dynamic_bot_token: + if self._default_bot_token_cache == token: + self._default_bot_token_cache = None + if self._resolved_default_token.get() == token: + self._resolved_default_token.set(None) def _get_web_client_for_token(self, token: str) -> Any: """Return a synchronous ``slack_sdk.WebClient`` for *token*, cached. diff --git a/tests/test_slack_dynamic_token_and_verifier.py b/tests/test_slack_dynamic_token_and_verifier.py index b762f79..0c74f9a 100644 --- a/tests/test_slack_dynamic_token_and_verifier.py +++ b/tests/test_slack_dynamic_token_and_verifier.py @@ -188,9 +188,10 @@ def test_sync_current_token_with_sync_resolver_invokes_resolver(self): resolvers too — single-workspace apps using a sync resolver for secret rotation could not read ``current_token`` / ``web_client`` outside a webhook context until an async path had primed the cache. - ``_get_token`` now invokes the sync resolver directly and primes - ``_default_bot_token_cache``; the async-resolver-in-sync-context - case still raises (see + ``_get_token`` now invokes the sync resolver directly — fresh on + every call, to honor the rotation contract on + :attr:`SlackAdapterConfig.bot_token`. The + async-resolver-in-sync-context case still raises (see ``test_sync_current_token_with_async_resolver_raises`` below). """ @@ -199,9 +200,12 @@ def resolver() -> str: adapter = SlackAdapter(SlackAdapterConfig(signing_secret="s", bot_token=resolver)) assert adapter.current_token == "xoxb-resolved" - # And the resolved token must be cached in the same slot the async - # path writes, so subsequent sync reads don't re-invoke. - assert adapter._default_bot_token_cache == "xoxb-resolved" + # Dynamic resolvers must NOT prime ``_default_bot_token_cache``; + # caching would suppress subsequent resolver calls and break the + # rotation contract. Rotation behavior is pinned by + # ``test_sync_callable_invoked_fresh_each_access`` in + # tests/test_slack_web_client.py. + assert adapter._default_bot_token_cache is None def test_sync_current_token_with_async_resolver_raises(self): """Async resolvers still cannot be awaited from the sync property.""" diff --git a/tests/test_slack_web_client.py b/tests/test_slack_web_client.py index 83b2297..421927d 100644 --- a/tests/test_slack_web_client.py +++ b/tests/test_slack_web_client.py @@ -256,36 +256,53 @@ class TestWebClientSyncResolver: cache cases — sync callables (used e.g. for lazy secret-manager loads or rotation) raised ``AuthenticationError`` from ``web_client`` outside any webhook / ContextVar scope, so proactive sends were impossible until an - async path had primed the cache. The new sync-callable branch invokes - the resolver and primes ``_default_bot_token_cache`` in the same way - ``_resolve_default_token`` does on the async path. + async path had primed the cache. The sync-callable branch invokes the + resolver fresh on every access to honor the rotation contract documented + on :attr:`SlackAdapterConfig.bot_token`. """ - def test_sync_callable_resolves_and_caches(self): - """A sync ``bot_token`` callable is invoked on first access and cached. + def test_sync_callable_invoked_fresh_each_access(self): + """Sync ``bot_token`` callables are invoked on *every* sync read. - Load-bearing: with the fix reverted the first access raises - ``AuthenticationError`` instead of returning the resolved token. + Load-bearing for two contracts: + + 1. The bot_token contract in ``types.py`` ("called on each use to + support rotation"). + 2. The audit fix that made sync resolvers reachable from + ``web_client`` at all (reverting it makes the first access raise). + + If the resolver were cached after the first call, a rotating + secret-manager-backed resolver would silently freeze on the + original value. """ + tokens = iter(["xoxb-sync-1", "xoxb-sync-2", "xoxb-sync-3"]) calls = {"n": 0} def resolver() -> str: calls["n"] += 1 - return "xoxb-sync-resolved" + return next(tokens) adapter = SlackAdapter(SlackAdapterConfig(signing_secret=_SECRET, bot_token=resolver)) first = adapter.web_client - assert first.token == "xoxb-sync-resolved" + assert first.token == "xoxb-sync-1" assert calls["n"] == 1 - # Second access must hit the cache and not re-invoke the resolver — - # matches the cache semantics of the async ``_resolve_default_token`` - # path (which writes ``_default_bot_token_cache``). + # Second access must re-invoke the resolver and see the rotated + # token — caching would break rotation. second = adapter.web_client - assert second is first - assert calls["n"] == 1 - assert adapter._default_bot_token_cache == "xoxb-sync-resolved" + assert second.token == "xoxb-sync-2" + assert calls["n"] == 2 + # WebClient cache is keyed by token; a new token yields a new client. + assert second is not first + + third = adapter.current_token + assert third == "xoxb-sync-3" + assert calls["n"] == 3 + + # Crucially, the dynamic-resolver path must NOT prime the + # process-wide cache — that would suppress future resolver calls. + assert adapter._default_bot_token_cache is None def test_async_callable_in_sync_context_raises(self): """``async def`` resolvers cannot be awaited from the sync property.""" @@ -356,3 +373,75 @@ def resolver() -> str: with pytest.raises(AuthenticationError): _ = adapter.web_client assert adapter._default_bot_token_cache is None + + +class TestInvalidateClientClearsTokenCaches: + """Pin that ``_invalidate_client`` clears the resolved-token caches for + dynamic-resolver configs. + + Before the fix, ``_invalidate_client`` only evicted the WebClient and + AsyncWebClient caches — the next ``_get_token`` would still return the + revoked token from ``_default_bot_token_cache`` (or the per-request + ContextVar), so a 401-driven invalidation just rebuilt clients around the + same revoked credential. + """ + + def test_invalidate_clears_default_cache_for_dynamic_resolver(self): + """Async-resolver path: after invalidation the cache is cleared. + + Load-bearing: with the fix reverted, ``_default_bot_token_cache`` + still equals the revoked token after invalidation. + """ + + async def resolver() -> str: + return "xoxb-revoked" + + adapter = SlackAdapter(SlackAdapterConfig(signing_secret=_SECRET, bot_token=resolver)) + # Simulate the async path having already primed the cache. + adapter._default_bot_token_cache = "xoxb-revoked" + + adapter._invalidate_client("xoxb-revoked") + + assert adapter._default_bot_token_cache is None + + def test_invalidate_clears_per_request_token_for_dynamic_resolver(self): + """ContextVar-primed per-request token is cleared on invalidation.""" + + async def resolver() -> str: + return "xoxb-revoked" + + adapter = SlackAdapter(SlackAdapterConfig(signing_secret=_SECRET, bot_token=resolver)) + adapter._resolved_default_token.set("xoxb-revoked") + + adapter._invalidate_client("xoxb-revoked") + + assert adapter._resolved_default_token.get() is None + + def test_invalidate_static_string_does_not_clear_token_cache(self): + """Static-string ``bot_token`` configs have no refresh path. + + Clearing the cache for a static-string config would just make every + subsequent sync access raise, with no way to recover. The fix + intentionally guards on ``_is_dynamic_bot_token`` to preserve the + static-string fast path. + """ + adapter = SlackAdapter(SlackAdapterConfig(signing_secret=_SECRET, bot_token="xoxb-static")) + assert adapter._default_bot_token_cache == "xoxb-static" + + adapter._invalidate_client("xoxb-static") + + # Client caches evicted but token cache retained. + assert adapter._default_bot_token_cache == "xoxb-static" + + def test_invalidate_different_token_does_not_clear_cache(self): + """Only invalidation of the *currently cached* token clears it.""" + + async def resolver() -> str: + return "xoxb-current" + + adapter = SlackAdapter(SlackAdapterConfig(signing_secret=_SECRET, bot_token=resolver)) + adapter._default_bot_token_cache = "xoxb-current" + + adapter._invalidate_client("xoxb-some-other-token") + + assert adapter._default_bot_token_cache == "xoxb-current" From eff1a007ca5b8abaf5fe9ab90a82fc39437eb82a Mon Sep 17 00:00:00 2001 From: Claude Date: Mon, 1 Jun 2026 19:15:34 +0000 Subject: [PATCH 6/6] chore(tests): address CodeRabbit duplicate-test + iter-StopIteration findings MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit CodeRabbit review on the latest #127 HEAD surfaced two test-quality issues: 1. ``TestWebClientAsyncResolver.test_unresolved_async_resolver_raises`` duplicated ``TestWebClientSyncResolver.test_async_callable_in_sync_context_raises`` (same async-resolver-in-sync-context path, but the latter is stronger — it also validates the error message wording). Removed the redundant wrapper class to honor this repo's "no duplicate tests" CLAUDE.md rule. 2. Rotation pin used ``tokens = iter(["xoxb-sync-1", ...])`` which would ``StopIteration`` if the test grew to a 4th access. Switched to ``f"xoxb-sync-{calls['n']}"`` so the resolver scales with call count; existing assertions on the literal values still hold. 68 tests still pass under ``pytest -W error::RuntimeWarning``. https://claude.ai/code/session_01FyMxQn2BEAzmwKS1GZczKj --- tests/test_slack_web_client.py | 26 ++++++-------------------- 1 file changed, 6 insertions(+), 20 deletions(-) diff --git a/tests/test_slack_web_client.py b/tests/test_slack_web_client.py index 421927d..be526f2 100644 --- a/tests/test_slack_web_client.py +++ b/tests/test_slack_web_client.py @@ -226,26 +226,10 @@ def test_raises_without_context_in_multi_workspace_mode(self): _ = adapter.client -# --------------------------------------------------------------------------- -# Async resolver: not-yet-resolved default token raises (sync property) -# --------------------------------------------------------------------------- - - -class TestWebClientAsyncResolver: - def test_unresolved_async_resolver_raises(self): - """A callable ``bot_token`` that has not run yet cannot resolve sync.""" - - async def resolver() -> str: - return "xoxb-async-resolved" - - adapter = SlackAdapter(SlackAdapterConfig(signing_secret=_SECRET, bot_token=resolver)) - - with pytest.raises(AuthenticationError): - _ = adapter.web_client - - # --------------------------------------------------------------------------- # Sync resolver: invoked directly from ``web_client`` (Codex P2 fix) +# Async resolver raising from the sync property is covered by +# ``TestWebClientSyncResolver.test_async_callable_in_sync_context_raises``. # --------------------------------------------------------------------------- @@ -275,12 +259,14 @@ def test_sync_callable_invoked_fresh_each_access(self): secret-manager-backed resolver would silently freeze on the original value. """ - tokens = iter(["xoxb-sync-1", "xoxb-sync-2", "xoxb-sync-3"]) calls = {"n": 0} def resolver() -> str: + # ``f"xoxb-sync-{n}"`` rather than ``next(iter([...]))`` so the + # test is robust if assertions evolve to add a 4th+ access — a + # plain iterator would StopIteration instead of failing cleanly. calls["n"] += 1 - return next(tokens) + return f"xoxb-sync-{calls['n']}" adapter = SlackAdapter(SlackAdapterConfig(signing_secret=_SECRET, bot_token=resolver))