feat(slack): expose web_client property on SlackAdapter (#98) (#127)

* feat(slack): expose web_client property on SlackAdapter (#98)

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

* 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

* fix(slack): invoke sync token resolvers in web_client / _get_token (codex review)

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.

* fix(slack): close orphan coroutine before raising awaitable-resolver error (audit)

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

* fix(slack): honor bot_token rotation contract in sync _get_token + invalidation

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

* chore(tests): address CodeRabbit duplicate-test + iter-StopIteration findings

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

---------

Co-authored-by: Claude <noreply@anthropic.com>

Author: patrick-chinchill

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
@@ -410,6 +411,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
@@ -481,6 +488,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
@@ -608,6 +625,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
     # ------------------------------------------------------------------
@@ -620,34 +692,65 @@ 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:
             return ctx.token
         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:
-            # 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.
+        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",
-                "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",
@@ -758,8 +861,40 @@ 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.
+
+        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

tests/test_slack_dynamic_token_and_verifier.py

@@ -181,18 +181,41 @@ 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 — 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).
+        """
 
         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"
+        # 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."""
+
+        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,20 +339,26 @@ 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
         ``self._resolved_default_token.set(token)``, assign
         ``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