feat(slack): expose web_client property on SlackAdapter (#98)#127
feat(slack): expose web_client property on SlackAdapter (#98)#127patrick-chinchill wants to merge 6 commits into
Conversation
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
|
Warning Review limit reached
More reviews will be available in 54 minutes and 48 seconds. Learn how PR review limits work. Your organization has run out of usage credits. Purchase more in the billing tab. ⌛ How to resolve this issue?After more reviews become available, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans include higher PR review limits than trial, open-source, and free plans. In all cases, reviews become available again over time. During sustained high-volume PR review activity, CodeRabbit may temporarily slow when the next review becomes available. Please see our Fair Usage Limits Policy for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughThis PR adds synchronous ChangesSlack Adapter Synchronous WebClient Feature
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request adds a synchronous web_client property and a deprecated client alias to the SlackAdapter to provide direct access to a synchronous slack_sdk.WebClient instance, backed by a new cache and covered by comprehensive unit tests. The review feedback points out that the newly introduced _web_client_cache is not cleared when _invalidate_client is called, which could leave stale or revoked clients in the cache, and suggests updating the invalidation method to clear both caches.
| """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: |
There was a problem hiding this comment.
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)![github-code-quality[bot]](https://avatars.githubusercontent.com/u/9919?s=60&v=4){kind=small}
…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
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
|
@codex review Generated by Claude Code |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: a7f5b53957
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| 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()) |
There was a problem hiding this comment.
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 👍 / 👎.
…odex 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.
|
@GHCodex review Generated by Claude Code |
…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
|
@GHCodex review Generated by Claude Code |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 41c8f3168a
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| "slack", | ||
| "Bot token resolver returned an empty or non-string value.", | ||
| ) | ||
| self._default_bot_token_cache = resolved |
There was a problem hiding this comment.
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 👍 / 👎.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/chat_sdk/adapters/slack/adapter.py`:
- Around line 844-847: _invalidate_client currently only removes entries from
_client_cache and _web_client_cache, but leaves the stale token in
_default_bot_token_cache and _resolved_default_token so subsequent calls to
_get_token will return the revoked token; update _invalidate_client to also pop
the token from _default_bot_token_cache and clear/reset _resolved_default_token
(and any resolver-cached state) for the given token so that _get_token will be
forced to re-resolve and not recreate clients with a revoked token; reference
the _invalidate_client method and the _default_bot_token_cache and
_resolved_default_token fields to locate where to add the additional cache
invalidation and reset logic.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro Plus
Run ID: b15ce462-bb98-4d5a-b9ea-384b45dc8720
📒 Files selected for processing (3)
src/chat_sdk/adapters/slack/adapter.pytests/test_slack_dynamic_token_and_verifier.pytests/test_slack_web_client.py
…validation 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
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
tests/test_slack_web_client.py (1)
235-244: 🛠️ Refactor suggestion | 🟠 Major | ⚡ Quick winDuplicate test: async resolver sync-access already covered.
This test verifies the same behavior as
TestWebClientSyncResolver.test_async_callable_in_sync_context_raises(lines 307-323): both confirm that an async resolver raisesAuthenticationErrorwhen accessed from the synchronousweb_clientproperty. The later test is more comprehensive (validates error message content), making this one redundant. As per coding guidelines, no two tests should verify the same thing to avoid inflating test counts without catching more bugs.♻️ Proposed fix: remove redundant test
Remove
TestWebClientAsyncResolver.test_unresolved_async_resolver_raisesentirely; the behavior is already validated bytest_async_callable_in_sync_context_raiseswith stronger assertions.-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) -# ---------------------------------------------------------------------------🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/test_slack_web_client.py` around lines 235 - 244, Remove the redundant test function test_unresolved_async_resolver_raises from TestWebClientAsyncResolver (the async resolver/sync-access case) because the behavior is already covered by TestWebClientSyncResolver.test_async_callable_in_sync_context_raises which includes stronger assertions; delete the entire test_unresolved_async_resolver_raises function (including its inner async resolver and adapter instantiation) to avoid duplicate assertions and inflated test counts.
🧹 Nitpick comments (1)
tests/test_slack_web_client.py (1)
278-283: 💤 Low valueConsider more robust token iteration.
The iterator will raise
StopIterationif the test logic changes and adds a 4th access. While the test is correct as written (exactly 3 accesses), using a cycling approach or count-limited generator would provide clearer failures if the test evolves.♻️ Proposed alternative: cycle or count-based resolver
- tokens = iter(["xoxb-sync-1", "xoxb-sync-2", "xoxb-sync-3"]) calls = {"n": 0} def resolver() -> str: calls["n"] += 1 - return next(tokens) + return f"xoxb-sync-{calls['n']}"🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/test_slack_web_client.py` around lines 278 - 283, The current resolver uses a simple iterator "tokens" and will raise StopIteration unexpectedly if accessed more than three times; update the resolver used in tests to be robust by replacing the plain iterator with a cycling or count-limited approach (e.g., use itertools.cycle over ["xoxb-sync-1","xoxb-sync-2","xoxb-sync-3"] or implement a resolver that returns tokens[calls["n"] % 3] and increments calls["n"]) so extra accesses either repeat predictably or produce a clear, controlled error; modify the "tokens", "calls", and "resolver" definitions accordingly to ensure stable test behavior if access count changes.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@tests/test_slack_web_client.py`:
- Around line 235-244: Remove the redundant test function
test_unresolved_async_resolver_raises from TestWebClientAsyncResolver (the async
resolver/sync-access case) because the behavior is already covered by
TestWebClientSyncResolver.test_async_callable_in_sync_context_raises which
includes stronger assertions; delete the entire
test_unresolved_async_resolver_raises function (including its inner async
resolver and adapter instantiation) to avoid duplicate assertions and inflated
test counts.
---
Nitpick comments:
In `@tests/test_slack_web_client.py`:
- Around line 278-283: The current resolver uses a simple iterator "tokens" and
will raise StopIteration unexpectedly if accessed more than three times; update
the resolver used in tests to be robust by replacing the plain iterator with a
cycling or count-limited approach (e.g., use itertools.cycle over
["xoxb-sync-1","xoxb-sync-2","xoxb-sync-3"] or implement a resolver that returns
tokens[calls["n"] % 3] and increments calls["n"]) so extra accesses either
repeat predictably or produce a clear, controlled error; modify the "tokens",
"calls", and "resolver" definitions accordingly to ensure stable test behavior
if access count changes.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 5512cb58-28a0-425c-89e9-135df1d2f556
📒 Files selected for processing (3)
src/chat_sdk/adapters/slack/adapter.pytests/test_slack_dynamic_token_and_verifier.pytests/test_slack_web_client.py
🚧 Files skipped from review as they are similar to previous changes (2)
- src/chat_sdk/adapters/slack/adapter.py
- tests/test_slack_dynamic_token_and_verifier.py
…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
2 participants
What's ported
Worklist item P1-3 (tracking issue #98): expose direct
WebClientaccess on the Slack adapter.SlackAdapter.web_client— a synchronousslack_sdk.WebClientbound to the bot token for the current request context (multi-workspace) or the configured default token (single-workspace). For any Slack Web API call not covered by the adapter's high-level methods (e.g.adapter.web_client.pins_add(...)).SlackAdapter._get_web_client_for_token(token)— direct port of upstream'sgetClientForToken: one cachedWebClientper distinct token. Kept separate from the existing async_client_cache(which holdsAsyncWebClientinstances used by the adapter's own API calls) because the sync and async client types are not interchangeable.slack_sdkimport stays deferred (optional dependency).SlackAdapter.client— deprecated one-release alias delegating toweb_client; emitsDeprecationWarning.Token resolution (3-level resolver, matches upstream + existing pattern)
web_clientresolves via the existing sync_get_token():with_bot_token/with_bot_token_async)bot_tokenconfigAuthenticationErrorUses the existing token ContextVar (
_request_context/_resolved_default_token) from the dynamic-bot-token work — noorfallbacks; resolution isis not None-based throughout the resolver it delegates to.Upstream refs
8366b8b(PR #471) — feat(slack): expose direct WebClient access viaadapter.client2279f1d(PR #472) — revertfdebde7(PR #476) — reapply2f108bd(PR #478) — renameadapter.client→adapter.webClient(Python:web_client)Net effect: the Slack adapter exposes a
WebClient, withclientretained as a deprecated alias.Tests
tests/test_slack_web_client.pymirrors upstream'swebClient getterdescribe block:web_clientreturns a client bound to the configured token; same instance cached per token; cache keyed by resolved token.clientalias returns the same object asweb_client, emits exactly oneDeprecationWarning;web_clientitself emits none.with_bot_token, and overrides the static default within that scope.AuthenticationErroron bothweb_clientandclient.bot_token→ raisesAuthenticationErrorfrom the sync property.Follow-ups (out of scope for this PR)
The
2f108bdrename also addedadapter.octokit(GitHub) andadapter.linearClient(Linear) alongsidewebClient. Inchat-sdk-pythonthe GitHub and Linear adapters do not yet have a.clientproperty to rename, so those equivalents are deferred to separate PRs (not part of #98 / P1-3).Validation
All run from the worktree against
origin/main(3ba6456):uv run ruff check src/ tests/— All checks passeduv run ruff format --check src/ tests/— 196 files already formatteduv run python scripts/audit_test_quality.py— 0 hard failures (pre-existing cross-file duplicate warnings only; none reference the new test file)uv run pytest tests/ -q— 4057 passed, 3 skipped (baseline + 10 new tests; no regressions)uv run pytest tests/ -q -k slack— 631 passedRelease coordination
Intentionally no CHANGELOG / pyproject.toml version changes — the coordinating
0.4.29release-cut PR will add the CHANGELOG/version entry to avoid N-way CHANGELOG collisions across the worklist batch.https://claude.ai/code/session_01FyMxQn2BEAzmwKS1GZczKj
Generated by Claude Code
Summary by CodeRabbit
New Features
Deprecations
clientis a deprecated alias for the new WebClient property and now emits a deprecation warning.Bug Fixes
Tests