Skip to content
Open
Show file tree
Hide file tree
Changes from 4 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
132 changes: 127 additions & 5 deletions src/chat_sdk/adapters/slack/adapter.py
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -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
Expand Down Expand Up @@ -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())

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Badge Invoke synchronous token resolvers for web_client

When SlackAdapterConfig.bot_token is a synchronous callable and web_client is accessed before a webhook or current_token_async() has primed _default_bot_token_cache, this path calls _get_token(), which raises AuthenticationError instead of invoking the configured resolver. That leaves single-workspace apps that use a sync resolver for rotation or lazy secret loading unable to use the new direct WebClient outside a request context, even though the resolver can be evaluated synchronously here.

Useful? React with 👍 / 👎.


@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
# ------------------------------------------------------------------
Expand Down Expand Up @@ -623,13 +689,49 @@ 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):
# 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

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Badge Avoid pinning sync resolver tokens after first access

When bot_token is a synchronous resolver used for rotation, this assignment makes the first resolved value permanent for all later sync reads because _get_token() checks _default_bot_token_cache before calling the provider. The config contract in types.py says callable bot tokens are invoked on each use to support rotation, but after adapter.web_client or current_token is accessed once, subsequent accesses keep returning a WebClient for the stale token even if the resolver now returns a rotated token.

Useful? React with 👍 / 👎.

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",
Expand Down Expand Up @@ -742,6 +844,26 @@ 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)
Comment thread
coderabbitai[bot] marked this conversation as resolved.

def _get_web_client_for_token(self, token: str) -> Any:

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

The newly introduced self._web_client_cache is not cleared when _invalidate_client is called (e.g., during token revocation or authentication errors in _handle_slack_error). To prevent stale or revoked synchronous WebClient instances from remaining in the cache, please update _invalidate_client to also pop the token from self._web_client_cache:

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)
    self._client_cache.pop(token, None)
    self._web_client_cache.pop(token, None)

"""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
Expand Down
45 changes: 35 additions & 10 deletions tests/test_slack_dynamic_token_and_verifier.py
Original file line number Diff line number Diff line change
Expand Up @@ -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

Expand Down Expand Up @@ -316,20 +335,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

Expand Down
Loading
Loading