From 7a9638d232b197319776758e27a9f1c282e3ef25 Mon Sep 17 00:00:00 2001 From: patrick-chinchill Date: Fri, 19 Jun 2026 03:23:14 -0700 Subject: [PATCH 1/4] feat(slack): stable link-button action_id from core id (chat@4.31, 171657a) Consume the new core LinkButtonElement.id as the Slack link-button action_id, with ?? semantics: an explicit (even empty-string) id is used verbatim; only a missing/None id falls back to link-{url}. - core cards.py: LinkButton() factory + LinkButtonElement gain optional id - adapters/slack/cards.py (_convert_link_button_to_element): id ?? link-{url[:200]} - adapters/slack/blocks/__init__.py (_link_button_to_element): id ?? link-{url}, still truncated via _truncate_text(..., LIMITS.action_id) on the id path - adapters/slack/blocks/types.py: SlackLinkButtonElement gains id: NotRequired[str] Tests lock custom id verbatim at both sites, URL fallback at both, and id=='' -> action_id=='' (locks 'is not None', not 'or'). --- .../adapters/slack/blocks/__init__.py | 6 +- src/chat_sdk/adapters/slack/blocks/types.py | 1 + src/chat_sdk/adapters/slack/cards.py | 5 +- tests/test_cards.py | 29 +++++++ tests/test_slack_blocks_primitives.py | 87 +++++++++++++++++++ tests/test_slack_cards.py | 40 +++++++++ 6 files changed, 166 insertions(+), 2 deletions(-) diff --git a/src/chat_sdk/adapters/slack/blocks/__init__.py b/src/chat_sdk/adapters/slack/blocks/__init__.py index ebddffa1..660ab484 100644 --- a/src/chat_sdk/adapters/slack/blocks/__init__.py +++ b/src/chat_sdk/adapters/slack/blocks/__init__.py @@ -311,9 +311,13 @@ def _button_to_element(button: SlackButtonElement, convert_emoji: _EmojiConverte def _link_button_to_element(button: SlackLinkButtonElement, convert_emoji: _EmojiConverter) -> dict[str, object]: + # `??` semantics: an explicit (even empty-string) id is used verbatim; + # only a missing/None id falls back to the URL-derived action_id. + button_id = button.get("id") + action_id = button_id if button_id is not None else f"link-{button['url']}" return _compact( { - "action_id": _truncate_text(f"link-{button['url']}", LIMITS.action_id), + "action_id": _truncate_text(action_id, LIMITS.action_id), "style": _map_button_style(button.get("style")), "text": _plain_text(button["label"], convert_emoji, LIMITS.button_text), "type": "button", diff --git a/src/chat_sdk/adapters/slack/blocks/types.py b/src/chat_sdk/adapters/slack/blocks/types.py index 2649261e..e6f37286 100644 --- a/src/chat_sdk/adapters/slack/blocks/types.py +++ b/src/chat_sdk/adapters/slack/blocks/types.py @@ -63,6 +63,7 @@ class SlackLinkButtonElement(TypedDict): type: Literal["link-button"] label: str url: str + id: NotRequired[str] style: NotRequired[SlackButtonStyle] diff --git a/src/chat_sdk/adapters/slack/cards.py b/src/chat_sdk/adapters/slack/cards.py index a3a65717..29ec6ee2 100644 --- a/src/chat_sdk/adapters/slack/cards.py +++ b/src/chat_sdk/adapters/slack/cards.py @@ -229,6 +229,9 @@ def _convert_button_to_element(button: ButtonElement) -> SlackButtonElement_: def _convert_link_button_to_element(button: LinkButtonElement) -> SlackLinkButtonElement_: """Convert a LinkButtonElement to a Slack link button element.""" url = button.get("url", "") + # `??` semantics: an explicit (even empty-string) id is used verbatim; + # only a missing/None id falls back to the URL-derived action_id. + button_id = button.get("id") element: SlackLinkButtonElement_ = { "type": "button", "text": { @@ -236,7 +239,7 @@ def _convert_link_button_to_element(button: LinkButtonElement) -> SlackLinkButto "text": convert_emoji(button.get("label", "")), "emoji": True, }, - "action_id": f"link-{url[:200]}", + "action_id": button_id if button_id is not None else f"link-{url[:200]}", "url": url, } diff --git a/tests/test_cards.py b/tests/test_cards.py index 5d7e042b..fcf8d1f5 100644 --- a/tests/test_cards.py +++ b/tests/test_cards.py @@ -14,6 +14,35 @@ from chat_sdk.shared.card_utils import escape_table_cell, render_gfm_table +class TestLinkButtonFactory: + """``LinkButton`` factory — including the optional ``id`` (chat@4.31).""" + + def test_minimal(self): + assert LinkButton(url="https://example.com", label="Open") == { + "type": "link-button", + "url": "https://example.com", + "label": "Open", + } + + def test_includes_id_when_provided(self): + # chat@4.31: ``id`` is an optional action identifier emitted by + # platforms that report link clicks. + assert LinkButton(url="https://example.com", label="Open", id="open-btn") == { + "type": "link-button", + "url": "https://example.com", + "label": "Open", + "id": "open-btn", + } + + def test_omits_id_key_when_none(self): + # Absent id must NOT add an ``id`` key (so adapter ``??`` fallbacks fire). + assert "id" not in LinkButton(url="https://example.com", label="Open") + + def test_empty_string_id_is_kept(self): + # An explicit empty-string id is kept (locks ``is not None``, not truthy). + assert LinkButton(url="https://example.com", label="Open", id="")["id"] == "" + + class TestIsCardElement: """Tests for is_card_element.""" diff --git a/tests/test_slack_blocks_primitives.py b/tests/test_slack_blocks_primitives.py index f37b3343..df23a71c 100644 --- a/tests/test_slack_blocks_primitives.py +++ b/tests/test_slack_blocks_primitives.py @@ -14,6 +14,7 @@ from typing import Any from chat_sdk.adapters.slack.blocks import ( + LIMITS, answered_slack_input_blocks, build_slack_freeform_view, card_to_block_kit, @@ -192,6 +193,92 @@ def test_converts_actions(self) -> None: "type": "actions", } + def test_link_button_uses_custom_action_id(self) -> None: + # chat@4.31 (171657a): an explicit ``id`` on a link-button is used as + # the action_id verbatim (still subject to the action_id length limit). + blocks = card_to_slack_blocks( + _card( + [ + { + "children": [ + { + "id": "agent_slack_auth_signin", + "label": "Sign in", + "type": "link-button", + "url": "https://vercel.com/oauth/authorize", + }, + ], + "type": "actions", + }, + ] + ) + ) + assert blocks[0]["elements"][0]["action_id"] == "agent_slack_auth_signin" + + def test_link_button_falls_back_to_url_action_id(self) -> None: + # No ``id`` → action_id falls back to the ``link-{url}`` form. + blocks = card_to_slack_blocks( + _card( + [ + { + "children": [ + { + "label": "Docs", + "type": "link-button", + "url": "https://example.com/docs", + }, + ], + "type": "actions", + }, + ] + ) + ) + assert blocks[0]["elements"][0]["action_id"] == "link-https://example.com/docs" + + def test_link_button_empty_id_is_used_verbatim(self) -> None: + # ``??`` semantics (NOT ``or``): an explicit empty-string id is used + # as-is and does NOT fall back to ``link-{url}``. Locks ``is not None``. + blocks = card_to_slack_blocks( + _card( + [ + { + "children": [ + { + "id": "", + "label": "Docs", + "type": "link-button", + "url": "https://example.com/docs", + }, + ], + "type": "actions", + }, + ] + ) + ) + assert blocks[0]["elements"][0]["action_id"] == "" + + def test_link_button_custom_id_truncated_to_action_id_limit(self) -> None: + # The id path still goes through ``_truncate_text(..., LIMITS.action_id)``. + long_id = "x" * (LIMITS.action_id + 50) + blocks = card_to_slack_blocks( + _card( + [ + { + "children": [ + { + "id": long_id, + "label": "Docs", + "type": "link-button", + "url": "https://example.com/docs", + }, + ], + "type": "actions", + }, + ] + ) + ) + assert blocks[0]["elements"][0]["action_id"] == "x" * LIMITS.action_id + def test_limits_action_elements_and_select_options(self) -> None: blocks = card_to_slack_blocks( _card( diff --git a/tests/test_slack_cards.py b/tests/test_slack_cards.py index 0a169584..8189e1d2 100644 --- a/tests/test_slack_cards.py +++ b/tests/test_slack_cards.py @@ -187,6 +187,46 @@ def test_link_buttons(self): assert el["text"] == {"type": "plain_text", "text": "View Docs", "emoji": True} assert el["url"] == "https://example.com/docs" assert el["style"] == "primary" + # No explicit id → action_id falls back to the URL-derived form. + assert el["action_id"] == "link-https://example.com/docs" + + def test_link_button_uses_custom_action_id(self): + # chat@4.31 (171657a): an explicit ``id`` is used verbatim as the + # action_id, so platforms that report link clicks get a stable id. + blocks = card_to_block_kit( + Card( + children=[ + Actions( + [ + LinkButton( + id="agent_slack_auth_signin", + url="https://vercel.com/oauth/authorize", + label="Sign in", + ), + ] + ), + ] + ) + ) + el = blocks[0]["elements"][0] + assert el["action_id"] == "agent_slack_auth_signin" + + def test_link_button_empty_id_is_used_verbatim(self): + # ``??`` semantics (NOT ``or``): an explicit empty-string id is used + # as-is and does NOT fall back to ``link-{url}``. Locks ``is not None``. + blocks = card_to_block_kit( + Card( + children=[ + Actions( + [ + LinkButton(id="", url="https://example.com/docs", label="View Docs"), + ] + ), + ] + ) + ) + el = blocks[0]["elements"][0] + assert el["action_id"] == "" def test_fields(self): blocks = card_to_block_kit( From a6b29b07760579e131435b1e3754d4be2a901658 Mon Sep 17 00:00:00 2001 From: patrick-chinchill Date: Fri, 19 Jun 2026 03:23:37 -0700 Subject: [PATCH 2/4] feat(slack): forward web_client_options to both WebClients (chat@4.31, 8336a3e) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit SlackAdapterConfig gains web_client_options: dict[str, Any] | None, spread into BOTH client construction sites — the default AsyncWebClient (_get_client) and the per-token sync WebClient (_get_web_client_for_token). DIVERGENCE: maps to slack_sdk WebClient kwargs (timeout in seconds, retry_handlers), not @slack/web-api axios options — slack_sdk has no 1:1 retryConfig/rejectRateLimitedCalls. Documented in the config docstring and a docs/UPSTREAM_SYNC.md row (plus a jsx-runtime LinkButtonProps.id non-port row). Hazards: gate the spread on 'is not None' so an explicit {} still applies; deep-copy any nested headers dict per client (_web_client_kwargs) so cached per-token clients never share a mutable dict and caller input is never mutated. Tests: an option reaches the default client and a per-token client; header isolation across two tokens with no input mutation; empty {} still spreads. --- docs/UPSTREAM_SYNC.md | 2 + src/chat_sdk/adapters/slack/adapter.py | 29 +++++++- src/chat_sdk/adapters/slack/types.py | 17 +++++ tests/test_adapter_api_url_config.py | 96 ++++++++++++++++++++++++++ 4 files changed, 142 insertions(+), 2 deletions(-) diff --git a/docs/UPSTREAM_SYNC.md b/docs/UPSTREAM_SYNC.md index acd20445..ebec841c 100644 --- a/docs/UPSTREAM_SYNC.md +++ b/docs/UPSTREAM_SYNC.md @@ -651,6 +651,8 @@ stay explicit instead of being rediscovered in code review. | Slack legacy mrkdwn renderer (response_url surface only, post-#440) | `_node_to_mrkdwn` renders headings as `*bold*` and images as `{alt} ({url})` / bare URL | TS `nodeToMrkdwn` has no heading/image branches — both fall through to `defaultNodeToText`, dropping heading emphasis and image URLs | Pre-existing Python improvement; after vercel/chat#440 it affects only `to_response_url_text` (ephemeral edits via response_url). Preserves visual hierarchy and image URLs Slack would otherwise lose. | | Slack `api` primitives `send_slack_response_url` URL gate (vercel/chat#548) | `send_slack_response_url` (`slack/api/__init__.py`) calls `_assert_slack_response_url(url)` before POSTing — requires an `https://*.slack.com` URL (where Slack-issued `response_url`s always live) and raises `ValueError` for anything else | Upstream `api/client.ts` `sendResponseUrl` POSTs to whatever `response_url` it is handed, with no scheme/host validation | SSRF guard. The `response_url` reaching this primitive can originate from a parsed-but-unverified interaction payload; without a gate a crafted value could redirect the POST (which carries no bearer token but does echo SDK-controlled message content and trigger an arbitrary outbound request) to an attacker host. Enforces `CLAUDE.md`'s "Validate external URLs before requests (SSRF)" rule, mirroring the high-level adapter's `rehydrate_attachment` allowlist row above. Allowlist: scheme `https`, host `slack.com` or `*.slack.com`. | | Slack `api` primitives `fetch_slack_file` host allowlist (vercel/chat#548) | `fetch_slack_file` (`slack/api/__init__.py`) gates `url` through `is_trusted_slack_file_url` before forwarding the bot token, raising `ValueError` for untrusted hosts | Upstream `api/client.ts` `fetchFile` GETs the supplied URL with `Authorization: Bearer ` unconditionally | Token-leak guard. `fetch_slack_file` attaches the workspace bot token; a crafted `url_private` from a parsed file object could otherwise exfiltrate that token to an arbitrary host. `is_trusted_slack_file_url` requires scheme `https` and host in `{files.slack.com, slack.com, *.slack.com, *.slack-edge.com}` — the same allowlist the high-level adapter's `rehydrate_attachment` row uses for Slack. Enforces `CLAUDE.md`'s SSRF/URL-validation rule. | +| Slack `web_client_options` → slack_sdk `WebClient` kwargs (vercel/chat#8336a3e, chat@4.31) | `SlackAdapterConfig.web_client_options: dict[str, Any] \| None` is spread (gated on `is not None`, so an explicit `{}` still spreads as a no-op) into **both** WebClient construction sites — the default `AsyncWebClient` (`_get_client`) and the per-token sync `WebClient` (`_get_web_client_for_token`). The keys are **slack_sdk** `WebClient` constructor kwargs: `timeout` (int seconds), `retry_handlers` (a list of `slack_sdk.http_retry.RetryHandler`), `headers`, etc. Any nested `headers` dict is **deep-copied per client** (`_web_client_kwargs`) so cached per-token clients never share a mutable dict and the caller's input is never mutated. | Upstream `webClientOptions?: Omit` forwards to `@slack/web-api`'s axios-backed `WebClient`; its headline keys are `retryConfig` (a `retryPolicies.*` policy), `rejectRateLimitedCalls`, and `timeout` (ms). | No 1:1 mapping: `slack_sdk` has no `retryConfig`/`rejectRateLimitedCalls` (retry behavior is configured via `retry_handlers`) and its `timeout` is seconds, not ms. So the option bag maps to slack_sdk `WebClient` kwargs rather than axios options. Same intent (tune the underlying HTTP client the adapter doesn't otherwise expose) and same per-client header isolation. Documented inline in `slack/types.py` (`web_client_options` docstring) and `slack/adapter.py` (`_web_client_kwargs`). Regression coverage: `tests/test_adapter_api_url_config.py::TestSlackWebClientOptions`. | +| jsx-runtime `LinkButtonProps.id` prop (vercel/chat#171657a slice, chat@4.31) | Not ported | `LinkButtonProps` gains `id?: string`; `resolveJSXElement` forwards it to `LinkButton({ id })` | Covered by the existing "JSX Card/Modal elements" row — Python has no JSX runtime. The underlying capability ports cleanly: the `LinkButton()` builder accepts `id=` directly, the core `LinkButtonElement` carries `id`, and the Slack adapter consumes it as the link-button `action_id` (with `??` fallback to `link-{url}`). | ### Platform-specific gaps diff --git a/src/chat_sdk/adapters/slack/adapter.py b/src/chat_sdk/adapters/slack/adapter.py index 50d9ba0f..681f03a6 100644 --- a/src/chat_sdk/adapters/slack/adapter.py +++ b/src/chat_sdk/adapters/slack/adapter.py @@ -552,6 +552,31 @@ def __init__(self, config: SlackAdapterConfig | None = None) -> None: # synchronous ``web_client`` escape hatch. self._slack_api_url: str | None = (config.api_url or os.environ.get("SLACK_API_URL")) or None + # Extra kwargs forwarded to every slack_sdk client (default async, + # per-token async cache, and synchronous WebClient). Mirrors upstream + # ``this.webClientOptions = config.webClientOptions`` (vercel/chat + # 8336a3e). Stored as-is (may be ``None``); spread is gated on + # ``is not None`` so an explicit ``{}`` still spreads. See + # ``_web_client_kwargs`` for the per-client deep-copy of ``headers``. + self._web_client_options: dict[str, Any] | None = config.web_client_options + + def _web_client_kwargs(self) -> dict[str, Any]: + """Return a fresh copy of ``web_client_options`` for one client. + + Spreads the configured options (gated on ``is not None`` so an empty + ``{}`` still applies) and **deep-copies any nested ``headers`` dict**, + so cached per-token clients never share a mutable ``headers`` object + and the caller's input dict is never mutated. Mirrors upstream's + per-client ``{ ...headers ? { headers: { ...headers } } : {} }`` spread. + """ + if self._web_client_options is None: + return {} + kwargs = dict(self._web_client_options) + headers = self._web_client_options.get("headers") + if headers is not None: + kwargs["headers"] = dict(headers) + return kwargs + # ------------------------------------------------------------------ # Properties (Adapter protocol) # ------------------------------------------------------------------ @@ -868,7 +893,7 @@ def _get_client(self, token: str | None = None) -> Any: # rejects ``base_url=None`` (it requires a string), and an empty string # must fall back to the built-in default, so mirroring upstream's truthy # spread keeps the default otherwise. - client_kwargs: dict[str, Any] = {"token": resolved_token} + client_kwargs: dict[str, Any] = {**self._web_client_kwargs(), "token": resolved_token} if self._slack_api_url: client_kwargs["base_url"] = self._slack_api_url client = AsyncWebClient(**client_kwargs) @@ -916,7 +941,7 @@ def _get_web_client_for_token(self, token: str) -> Any: # Same truthy ``base_url`` rule as ``_get_client`` — pass the # override only when truthy so slack_sdk keeps its default. - web_client_kwargs: dict[str, Any] = {"token": token} + web_client_kwargs: dict[str, Any] = {**self._web_client_kwargs(), "token": token} if self._slack_api_url: web_client_kwargs["base_url"] = self._slack_api_url client = WebClient(**web_client_kwargs) diff --git a/src/chat_sdk/adapters/slack/types.py b/src/chat_sdk/adapters/slack/types.py index 0721f261..c308a740 100644 --- a/src/chat_sdk/adapters/slack/types.py +++ b/src/chat_sdk/adapters/slack/types.py @@ -49,6 +49,23 @@ class SlackAdapterConfig: # ``slackApiUrl`` (vercel/chat 6b17c60). Useful for proxies, Slack-API # mocks in tests, or Enterprise-routed deployments. api_url: str | None = None + # Extra keyword arguments forwarded to every ``slack_sdk`` client the + # adapter builds — the default ``AsyncWebClient``, the per-token async + # cache, and the synchronous ``WebClient`` escape hatch. Gated on + # ``is not None`` so an explicit empty ``{}`` still spreads (a no-op). + # + # DIVERGENCE (vercel/chat 8336a3e): upstream forwards ``webClientOptions`` + # to ``@slack/web-api``'s ``WebClient`` (an axios-backed client), so its + # most useful keys are ``retryConfig`` and ``timeout``. There is no 1:1 + # mapping in ``slack_sdk``: it has no ``retryConfig``/``rejectRateLimitedCalls`` + # — retry behavior is configured via ``retry_handlers`` (a list of + # ``slack_sdk.http_retry.RetryHandler``), and ``timeout`` is an int of + # seconds. So these map to **slack_sdk WebClient kwargs**, not axios options. + # Example: ``web_client_options={"timeout": 15, "retry_handlers": [...]}``. + # ``headers`` (a dict) is deep-copied per client so cached per-token clients + # never share a mutable dict and caller input is never mutated. See the + # ``webClientOptions`` divergence row in docs/UPSTREAM_SYNC.md. + web_client_options: dict[str, Any] | None = None # App-level token (xapp-...). Required when ``mode == "socket"``. app_token: str | None = None # Bot token (xoxb-...). Required for single-workspace mode. Omit for multi-workspace. diff --git a/tests/test_adapter_api_url_config.py b/tests/test_adapter_api_url_config.py index 841250d7..64ad504b 100644 --- a/tests/test_adapter_api_url_config.py +++ b/tests/test_adapter_api_url_config.py @@ -224,6 +224,102 @@ def test_real_slack_sdk_normalizes_base_url(self) -> None: assert client.base_url == "https://slack.proxy.example/api/" +# =========================================================================== +# Slack — webClientOptions spread into both client constructions (chat@4.31, 8336a3e) +# =========================================================================== + + +class TestSlackWebClientOptions: + """``web_client_options`` is forwarded to BOTH the default async client and + the per-token sync client. Mirrors upstream's index.test.ts webClientOptions + block. DIVERGENCE: maps to slack_sdk ``WebClient`` kwargs (``timeout``, + ``retry_handlers``), not ``@slack/web-api`` axios options. + """ + + def _make(self, **overrides: Any) -> SlackAdapter: + config = SlackAdapterConfig( + signing_secret=overrides.pop("signing_secret", "test-signing-secret"), + bot_token=overrides.pop("bot_token", "xoxb-default-token"), + **overrides, + ) + return SlackAdapter(config) + + def test_option_reaches_default_async_client(self, monkeypatch: pytest.MonkeyPatch) -> None: + # Upstream: "forwards webClientOptions to the internal WebClient". + _patch_slack_clients(monkeypatch) + adapter = self._make(web_client_options={"timeout": 15}) + client = adapter._get_client("xoxb-tok") + assert client.kwargs["timeout"] == 15 + assert client.kwargs["token"] == "xoxb-tok" + + def test_option_reaches_per_token_sync_client(self, monkeypatch: pytest.MonkeyPatch) -> None: + # Upstream: "forwards webClientOptions to token-bound WebClients". + _patch_slack_clients(monkeypatch) + adapter = self._make(bot_token=None, web_client_options={"timeout": 15}) + client = adapter._get_web_client_for_token("xoxb-context-token") + assert client.kwargs["timeout"] == 15 + assert client.kwargs["token"] == "xoxb-context-token" + + def test_none_options_spreads_nothing(self, monkeypatch: pytest.MonkeyPatch) -> None: + # The default (None) must add no extra kwargs at all. + _patch_slack_clients(monkeypatch) + adapter = self._make() + assert adapter._web_client_options is None + client = adapter._get_client("xoxb-tok") + assert client.kwargs == {"token": "xoxb-tok"} + + def test_empty_dict_options_still_spreads(self, monkeypatch: pytest.MonkeyPatch) -> None: + # Gate is ``is not None`` (not truthiness): an explicit empty ``{}`` is a + # valid no-op spread, not a fall-through. Locks ``is not None`` vs ``or``. + _patch_slack_clients(monkeypatch) + adapter = self._make(web_client_options={}) + assert adapter._web_client_options == {} + client = adapter._get_client("xoxb-tok") + assert client.kwargs == {"token": "xoxb-tok"} + + def test_headers_isolated_across_per_token_clients(self, monkeypatch: pytest.MonkeyPatch) -> None: + # Upstream: "isolates custom headers between token-bound WebClients". + # Each cached client must get its OWN headers dict so mutating one (e.g. + # the per-token Authorization header slack_sdk sets) cannot leak across + # tokens. We deep-copy ``headers`` per client to guarantee this. + _patch_slack_clients(monkeypatch) + headers = {"X-Test": "value"} + adapter = self._make(bot_token=None, web_client_options={"headers": headers}) + + first = adapter._get_web_client_for_token("xoxb-first") + second = adapter._get_web_client_for_token("xoxb-second") + + assert first.kwargs["headers"] == {"X-Test": "value"} + assert second.kwargs["headers"] == {"X-Test": "value"} + # Distinct dict objects, not a shared reference. + assert first.kwargs["headers"] is not second.kwargs["headers"] + + def test_caller_input_headers_not_mutated(self, monkeypatch: pytest.MonkeyPatch) -> None: + # The caller's original ``headers`` dict must never be mutated or + # aliased by the adapter. Upstream asserts the input ``headers`` is + # unchanged after building two clients. + _patch_slack_clients(monkeypatch) + headers = {"X-Test": "value"} + adapter = self._make(bot_token=None, web_client_options={"headers": headers}) + + first = adapter._get_web_client_for_token("xoxb-first") + adapter._get_web_client_for_token("xoxb-second") + + # The constructed client holds a copy, not the caller's object. + assert first.kwargs["headers"] is not headers + assert headers == {"X-Test": "value"} + + def test_option_reaches_both_client_kinds(self, monkeypatch: pytest.MonkeyPatch) -> None: + # The same option must land in BOTH the async default client and the + # sync per-token client (two distinct construction sites in the adapter). + _patch_slack_clients(monkeypatch) + adapter = self._make(web_client_options={"timeout": 42}) + async_client = adapter._get_client("xoxb-tok") + sync_client = adapter._get_web_client_for_token("xoxb-tok") + assert async_client.kwargs["timeout"] == 42 + assert sync_client.kwargs["timeout"] == 42 + + # =========================================================================== # Discord — api_url threaded into _discord_fetch # =========================================================================== From 30c68427adb8738901e0b318831393ecc3ab1e7e Mon Sep 17 00:00:00 2001 From: patrick-chinchill Date: Fri, 19 Jun 2026 03:23:48 -0700 Subject: [PATCH 3/4] fix(slack): don't rewrite @handles inside URLs as mentions (chat@4.31, a8bf99a) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit A bare @handle inside a URL (path, query string, or fragment) must NOT be rewritten into a <@handle> Slack mention, which corrupts the link. This is a confirmed pre-existing converter bug; the new regression tests fail on pre-fix code. In adapters/slack/format_converter.py: - widen the mention lookbehind (? (?]+) and _link_bare_mentions_outside_urls(), which only mention-links the slices outside URL spans - call it from BOTH substitution sites — _finalize (string surface) and the _node_to_mrkdwn text branch (mrkdwn surface); our port had split upstream's single finalize into two bare subs - apply re.ASCII so \w matches JS's ASCII \w exactly (Python \w is Unicode-aware) Tests mirror upstream markdown.test.ts plus an adversarial sweep: @handle in path/query/fragment, schemeless host, real mention after a URL, (cc @george), email + mailto preserved, URL at start/end, multiple URLs, already-wrapped mention not double-wrapped — on both the string and mrkdwn surfaces. --- .../adapters/slack/format_converter.py | 45 ++++++++- tests/test_slack_format.py | 95 +++++++++++++++++++ 2 files changed, 135 insertions(+), 5 deletions(-) diff --git a/src/chat_sdk/adapters/slack/format_converter.py b/src/chat_sdk/adapters/slack/format_converter.py index 5bf74b33..ebc78af5 100644 --- a/src/chat_sdk/adapters/slack/format_converter.py +++ b/src/chat_sdk/adapters/slack/format_converter.py @@ -27,9 +27,44 @@ ) # Match bare @mentions (e.g. "@george") to rewrite as Slack's `<@george>`. -# The lookbehind excludes `<` (already-formatted mentions like `<@U123>`) and -# any word character, so email addresses like `user@example.com` are left alone. -BARE_MENTION_REGEX = re.compile(r"(?`), +# - any word character (so email addresses like `user@example.com` are left +# alone), and +# - `/` (so a schemeless host path like `mastodon.social/@user` — which the +# URL matcher in `_link_bare_mentions_outside_urls` does NOT catch — is also +# preserved). Mirrors upstream's `(?` +# Slack mention, which would corrupt the link. Mirrors upstream's `URL_PATTERN`. +URL_REGEX = re.compile(r"\bhttps?://[^\s<>]+") + + +def _link_bare_mentions_outside_urls(text: str) -> str: + """Rewrite bare ``@handle`` mentions, but only outside ``http(s)`` URL spans. + + A bare ``@handle`` anywhere inside a URL (path, query string, or fragment) + is preserved verbatim; mention linking is applied only to the text slices + between (and after) URL spans. Mirrors upstream's + ``linkBareMentionsOutsideUrls`` (vercel/chat a8bf99a). The schemeless host + path case (``mastodon.social/@user``), which ``URL_REGEX`` does not match, + is additionally guarded by the ``/`` in ``BARE_MENTION_REGEX``'s lookbehind. + """ + result: list[str] = [] + last_index = 0 + for match in URL_REGEX.finditer(text): + start = match.start() + result.append(BARE_MENTION_REGEX.sub(r"<@\1>", text[last_index:start])) + result.append(match.group(0)) + last_index = match.end() + result.append(BARE_MENTION_REGEX.sub(r"<@\1>", text[last_index:])) + return "".join(result) class SlackFormatConverter(BaseFormatConverter): @@ -171,7 +206,7 @@ def extract_plain_text(self, platform_text: str) -> str: def _finalize(self, text: str) -> str: """Rewrite bare @mentions and normalize emoji placeholders for Slack.""" - return convert_emoji_placeholders(BARE_MENTION_REGEX.sub(r"<@\1>", text), "slack") + return convert_emoji_placeholders(_link_bare_mentions_outside_urls(text), "slack") def _ast_to_mrkdwn(self, ast: Root) -> str: """Render an AST to Slack's legacy mrkdwn (response_url surface only).""" @@ -190,7 +225,7 @@ def _node_to_mrkdwn(self, node: Content) -> str: if node_type == "text": value = node.get("value", "") - return BARE_MENTION_REGEX.sub(r"<@\1>", value) + return _link_bare_mentions_outside_urls(value) if node_type == "strong": content = "".join(self._node_to_mrkdwn(c) for c in children) diff --git a/tests/test_slack_format.py b/tests/test_slack_format.py index 16a9490c..7ef6d14c 100644 --- a/tests/test_slack_format.py +++ b/tests/test_slack_format.py @@ -225,6 +225,101 @@ def test_does_not_mangle_mailto_links(self): def test_converts_mentions_adjacent_to_non_word_punctuation(self): assert self.converter.to_slack_payload("(cc @george, @anne)") == {"text": "(cc <@george>, <@anne>)"} + # -- @mention-in-URL fix (chat@4.31, a8bf99a) -------------------------------- + # A bare ``@handle`` inside a URL (path/query/fragment) or a schemeless host + # path must NOT be rewritten into a ``<@handle>`` Slack mention, which would + # corrupt the link. These mirror upstream markdown.test.ts:189-230 and are + # regression guards for a confirmed pre-existing converter bug — they FAIL on + # pre-fix code (where ``@jkyang`` etc. were wrapped into ``<@jkyang>``). + # The string surface exercises ``_finalize``; the mrkdwn surface (via + # ``to_response_url_text``) exercises the ``_node_to_mrkdwn`` text branch. + + def test_does_not_mangle_mention_in_url_path_plain_string(self): + assert self.converter.to_slack_payload("See https://hackmd.io/@jkyang/B1W69XA-fe") == { + "text": "See https://hackmd.io/@jkyang/B1W69XA-fe" + } + + def test_does_not_mangle_mention_in_url_path_markdown(self): + assert self.converter.to_slack_payload({"markdown": "See https://mastodon.social/@user for updates"}) == { + "markdown_text": "See https://mastodon.social/@user for updates" + } + + def test_does_not_mangle_mention_in_url_query_string(self): + assert self.converter.to_slack_payload("Profile https://example.com/p?user=@george") == { + "text": "Profile https://example.com/p?user=@george" + } + + def test_does_not_mangle_mention_in_url_fragment(self): + assert self.converter.to_slack_payload("Jump https://example.com/docs#@george") == { + "text": "Jump https://example.com/docs#@george" + } + + def test_does_not_mangle_mention_in_schemeless_host_path(self): + # ``URL_REGEX`` does not match a schemeless host; the ``/`` in the + # mention lookbehind guards this case. + assert self.converter.to_slack_payload("See hackmd.io/@jkyang/abc") == {"text": "See hackmd.io/@jkyang/abc"} + + def test_still_rewrites_real_mention_after_url(self): + # A genuine mention AFTER a URL (in the trailing slice) is still linked. + assert self.converter.to_slack_payload("See https://hackmd.io/@jkyang/abc cc @george") == { + "text": "See https://hackmd.io/@jkyang/abc cc <@george>" + } + + # -- mrkdwn (node) surface: both substitution sites must carry the fix ------- + + def test_url_mention_preserved_on_mrkdwn_surface(self): + # ``_node_to_mrkdwn`` text branch must skip the URL span too. + assert ( + self.converter.to_response_url_text({"markdown": "See https://hackmd.io/@jkyang/abc"}) + == "See https://hackmd.io/@jkyang/abc" + ) + + def test_real_mention_after_url_rewritten_on_mrkdwn_surface(self): + assert ( + self.converter.to_response_url_text({"markdown": "See https://hackmd.io/@jkyang/abc cc @george"}) + == "See https://hackmd.io/@jkyang/abc cc <@george>" + ) + + # -- adversarial budget (docs/SELF_REVIEW.md) ------------------------------- + + def test_email_address_still_preserved_after_fix(self): + # The ``\w`` lookbehind still protects email local parts. + assert self.converter.to_slack_payload("Contact user@example.com for help") == { + "text": "Contact user@example.com for help" + } + + def test_mailto_link_still_preserved_after_fix(self): + assert self.converter.to_slack_payload("Email ") == { + "text": "Email " + } + + def test_cc_george_regression_guard(self): + # The original real-mention behavior must survive the URL-skip rewrite. + assert self.converter.to_slack_payload("(cc @george)") == {"text": "(cc <@george>)"} + + def test_url_at_start_of_text(self): + assert self.converter.to_slack_payload("https://example.com/@george done") == { + "text": "https://example.com/@george done" + } + + def test_url_at_end_of_text(self): + # Mention before the URL is rewritten; the @handle inside the URL is not. + assert self.converter.to_slack_payload("ping @anne https://example.com/@george") == { + "text": "ping <@anne> https://example.com/@george" + } + + def test_multiple_urls_with_mention_between(self): + assert self.converter.to_slack_payload("a https://x.io/@one then @two and https://y.io/@three") == { + "text": "a https://x.io/@one then <@two> and https://y.io/@three" + } + + def test_already_wrapped_mention_in_url_not_double_wrapped(self): + # An already-``<@>``-wrapped handle is left intact, and the URL handle is + # preserved — neither is double-wrapped. + assert self.converter.to_slack_payload("hi <@U123> see https://x.io/@bob") == { + "text": "hi <@U123> see https://x.io/@bob" + } + # --------------------------------------------------------------------------- # toPlainText From 0170d85b3ca827d14a7927ed5a4519b400be0897 Mon Sep 17 00:00:00 2001 From: patrick-chinchill Date: Fri, 19 Jun 2026 16:10:54 -0700 Subject: [PATCH 4/4] fix(cards): align LinkButton stable-id param to `id` (not `id_`), matching Button/Select MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The Wave-A Slack PR's rebase onto #153 surfaced an API inconsistency: #153 landed LinkButton's optional stable-id param as `id_`, but Button/Select/RadioSelect all use `id`. Align LinkButton to the `id` convention — it's keyword-only with no real builtin-shadow issue (Button already does this; ruff's lint config does not select flake8-builtins), and 0.4.31 is unreleased so nothing depends on `id_` yet. Also drops this branch's duplicate jsx-runtime non-parity row (keeps #153's canonical one) and its redundant core LinkButton tests (keeps #153's TestLinkButtonId). --- docs/UPSTREAM_SYNC.md | 4 +--- src/chat_sdk/cards.py | 15 ++++++++------- tests/test_cards.py | 37 ++++--------------------------------- 3 files changed, 13 insertions(+), 43 deletions(-) diff --git a/docs/UPSTREAM_SYNC.md b/docs/UPSTREAM_SYNC.md index ebec841c..5af0ea98 100644 --- a/docs/UPSTREAM_SYNC.md +++ b/docs/UPSTREAM_SYNC.md @@ -646,14 +646,12 @@ stay explicit instead of being rediscovered in code review. | `@chat-adapter/tests` adapter test kit (vercel/chat#470) | Not ported | New TS package with test utilities for adapter authors | Python already ships `chat_sdk.testing` (`MockAdapter`, `MockStateAdapter`, `create_test_message()`) covering the same surface for this repo's adapter tests; mirroring the TS kit verbatim would duplicate it. Revisit if upstream's kit grows capabilities ours lacks (e.g. recorded replay fixtures for third-party adapter authors). | | Teams modal-submit webhook options (vercel/chat#454 adapter-teams slice) | Not ported — the Python Teams adapter has no task-module/modal-submit flow (`handleTaskSubmit`/`processModalSubmit` are absent), so upstream's change passing `bridgeAdapter.getWebhookOptions(activity.id)` into `processModalSubmit` has no landing site | `TeamsAdapter.handleTaskSubmit` forwards webhook options so modal callbackUrl POSTs are registered with `waitUntil` | Pre-existing gap: Teams modals are unported. The Slack adapter already forwards options to `process_modal_submit`, so the new waitUntil plumbing is exercised there. Add the Teams call when Teams modal support lands. | | jsx-runtime `callbackUrl` props (vercel/chat#454 slice) | Not ported | `ButtonProps`/`ModalProps` gain `callbackUrl`; `resolveJSXElement` forwards it | Covered by the existing "JSX Card/Modal elements" row — Python has no JSX runtime; `Button()`/`Modal()` builders accept `callback_url` directly. | -| jsx-runtime `id` prop for link buttons (stable-id-for-link-buttons, chat@4.31.0 commit `171657a`) | Not ported | `LinkButtonProps` gains `id?`; `resolveJSXElement` forwards `id: props.id` | Covered by the existing "JSX Card/Modal elements" row — Python has no JSX runtime. The core half of the same commit (`LinkButton()` factory + `LinkButtonElement` `id`) **is** ported: the `LinkButton(id_=…)` builder accepts the optional stable identifier directly. | +| jsx-runtime `id` prop for link buttons (stable-id-for-link-buttons, chat@4.31.0 commit `171657a`) | Not ported | `LinkButtonProps` gains `id?`; `resolveJSXElement` forwards `id: props.id` | Covered by the existing "JSX Card/Modal elements" row — Python has no JSX runtime. The core half of the same commit (`LinkButton()` factory + `LinkButtonElement` `id`) **is** ported: the `LinkButton(id=…)` builder accepts the optional stable identifier directly (matching the `Button`/`Select` `id` convention). | | Transcripts API Python adaptations (vercel/chat#448) | `transcripts.delete()` returns a `DeleteResult` dataclass; misconfiguration raises `ValueError` (constructor/`AppendInput` guards, invalid duration) or `ChatError` (`chat.transcripts` accessor); guard messages name the Python kwarg (`options.user_key`); `DurationString` is a `str` alias validated at runtime by `_parse_duration` | Inline `{ deleted: number }`; generic `Error` for all of the above; template-literal `` `${number}${"s"\|"m"\|"h"\|"d"}` `` type | Port rules: typed dataclasses over raw dicts; repo error-type conventions (constructor misconfig → `ValueError`, runtime API misuse → `ChatError`) with upstream-matching message wording; Python has no template-literal types. Same shapes and values throughout. | | Slack legacy mrkdwn renderer (response_url surface only, post-#440) | `_node_to_mrkdwn` renders headings as `*bold*` and images as `{alt} ({url})` / bare URL | TS `nodeToMrkdwn` has no heading/image branches — both fall through to `defaultNodeToText`, dropping heading emphasis and image URLs | Pre-existing Python improvement; after vercel/chat#440 it affects only `to_response_url_text` (ephemeral edits via response_url). Preserves visual hierarchy and image URLs Slack would otherwise lose. | | Slack `api` primitives `send_slack_response_url` URL gate (vercel/chat#548) | `send_slack_response_url` (`slack/api/__init__.py`) calls `_assert_slack_response_url(url)` before POSTing — requires an `https://*.slack.com` URL (where Slack-issued `response_url`s always live) and raises `ValueError` for anything else | Upstream `api/client.ts` `sendResponseUrl` POSTs to whatever `response_url` it is handed, with no scheme/host validation | SSRF guard. The `response_url` reaching this primitive can originate from a parsed-but-unverified interaction payload; without a gate a crafted value could redirect the POST (which carries no bearer token but does echo SDK-controlled message content and trigger an arbitrary outbound request) to an attacker host. Enforces `CLAUDE.md`'s "Validate external URLs before requests (SSRF)" rule, mirroring the high-level adapter's `rehydrate_attachment` allowlist row above. Allowlist: scheme `https`, host `slack.com` or `*.slack.com`. | | Slack `api` primitives `fetch_slack_file` host allowlist (vercel/chat#548) | `fetch_slack_file` (`slack/api/__init__.py`) gates `url` through `is_trusted_slack_file_url` before forwarding the bot token, raising `ValueError` for untrusted hosts | Upstream `api/client.ts` `fetchFile` GETs the supplied URL with `Authorization: Bearer ` unconditionally | Token-leak guard. `fetch_slack_file` attaches the workspace bot token; a crafted `url_private` from a parsed file object could otherwise exfiltrate that token to an arbitrary host. `is_trusted_slack_file_url` requires scheme `https` and host in `{files.slack.com, slack.com, *.slack.com, *.slack-edge.com}` — the same allowlist the high-level adapter's `rehydrate_attachment` row uses for Slack. Enforces `CLAUDE.md`'s SSRF/URL-validation rule. | | Slack `web_client_options` → slack_sdk `WebClient` kwargs (vercel/chat#8336a3e, chat@4.31) | `SlackAdapterConfig.web_client_options: dict[str, Any] \| None` is spread (gated on `is not None`, so an explicit `{}` still spreads as a no-op) into **both** WebClient construction sites — the default `AsyncWebClient` (`_get_client`) and the per-token sync `WebClient` (`_get_web_client_for_token`). The keys are **slack_sdk** `WebClient` constructor kwargs: `timeout` (int seconds), `retry_handlers` (a list of `slack_sdk.http_retry.RetryHandler`), `headers`, etc. Any nested `headers` dict is **deep-copied per client** (`_web_client_kwargs`) so cached per-token clients never share a mutable dict and the caller's input is never mutated. | Upstream `webClientOptions?: Omit` forwards to `@slack/web-api`'s axios-backed `WebClient`; its headline keys are `retryConfig` (a `retryPolicies.*` policy), `rejectRateLimitedCalls`, and `timeout` (ms). | No 1:1 mapping: `slack_sdk` has no `retryConfig`/`rejectRateLimitedCalls` (retry behavior is configured via `retry_handlers`) and its `timeout` is seconds, not ms. So the option bag maps to slack_sdk `WebClient` kwargs rather than axios options. Same intent (tune the underlying HTTP client the adapter doesn't otherwise expose) and same per-client header isolation. Documented inline in `slack/types.py` (`web_client_options` docstring) and `slack/adapter.py` (`_web_client_kwargs`). Regression coverage: `tests/test_adapter_api_url_config.py::TestSlackWebClientOptions`. | -| jsx-runtime `LinkButtonProps.id` prop (vercel/chat#171657a slice, chat@4.31) | Not ported | `LinkButtonProps` gains `id?: string`; `resolveJSXElement` forwards it to `LinkButton({ id })` | Covered by the existing "JSX Card/Modal elements" row — Python has no JSX runtime. The underlying capability ports cleanly: the `LinkButton()` builder accepts `id=` directly, the core `LinkButtonElement` carries `id`, and the Slack adapter consumes it as the link-button `action_id` (with `??` fallback to `link-{url}`). | - ### Platform-specific gaps | Area | Python | TS | Rationale | diff --git a/src/chat_sdk/cards.py b/src/chat_sdk/cards.py index 22c70e81..6607eb59 100644 --- a/src/chat_sdk/cards.py +++ b/src/chat_sdk/cards.py @@ -290,7 +290,7 @@ def LinkButton( url: str, label: str, style: ButtonStyle | None = None, - id_: str | None = None, + id: str | None = None, ) -> LinkButtonElement: """Create a LinkButton element that opens a URL when clicked. @@ -298,14 +298,15 @@ def LinkButton( LinkButton(url="https://example.com", label="View Docs") - ``id_`` is an optional action identifier emitted by platforms that report - link clicks. Upstream sets ``id`` unconditionally and relies on - ``JSON.stringify`` dropping ``undefined``; in Python we only write the - key when it is provided so an unset id never serializes as ``null``. + ``id`` is an optional action identifier emitted by platforms that report + link clicks (matching the ``Button``/``Select`` ``id`` convention). Upstream + sets ``id`` unconditionally and relies on ``JSON.stringify`` dropping + ``undefined``; in Python we only write the key when it is provided so an + unset id never serializes as ``null``. """ element: LinkButtonElement = {"type": "link-button", "url": url, "label": label} - if id_ is not None: - element["id"] = id_ + if id is not None: + element["id"] = id if style is not None: element["style"] = style return element diff --git a/tests/test_cards.py b/tests/test_cards.py index fcf8d1f5..0246b480 100644 --- a/tests/test_cards.py +++ b/tests/test_cards.py @@ -14,35 +14,6 @@ from chat_sdk.shared.card_utils import escape_table_cell, render_gfm_table -class TestLinkButtonFactory: - """``LinkButton`` factory — including the optional ``id`` (chat@4.31).""" - - def test_minimal(self): - assert LinkButton(url="https://example.com", label="Open") == { - "type": "link-button", - "url": "https://example.com", - "label": "Open", - } - - def test_includes_id_when_provided(self): - # chat@4.31: ``id`` is an optional action identifier emitted by - # platforms that report link clicks. - assert LinkButton(url="https://example.com", label="Open", id="open-btn") == { - "type": "link-button", - "url": "https://example.com", - "label": "Open", - "id": "open-btn", - } - - def test_omits_id_key_when_none(self): - # Absent id must NOT add an ``id`` key (so adapter ``??`` fallbacks fire). - assert "id" not in LinkButton(url="https://example.com", label="Open") - - def test_empty_string_id_is_kept(self): - # An explicit empty-string id is kept (locks ``is not None``, not truthy). - assert LinkButton(url="https://example.com", label="Open", id="")["id"] == "" - - class TestIsCardElement: """Tests for is_card_element.""" @@ -253,7 +224,7 @@ class TestLinkButtonId: """ def test_id_written_when_provided(self): - btn = LinkButton(url="https://example.com/docs", label="Docs", id_="open-docs") + btn = LinkButton(url="https://example.com/docs", label="Docs", id="open-docs") assert btn["id"] == "open-docs" def test_no_id_key_when_omitted(self): @@ -264,13 +235,13 @@ def test_no_id_key_when_omitted(self): def test_empty_string_id_is_emitted(self): # Explicit empty string is distinct from unset and must survive - # (this is exactly why we use ``is not None`` and not ``id_ or ...``). - btn = LinkButton(url="https://example.com/docs", label="Docs", id_="") + # (this is exactly why we use ``is not None`` and not ``id or ...``). + btn = LinkButton(url="https://example.com/docs", label="Docs", id="") assert "id" in btn assert btn["id"] == "" def test_id_survives_wire_serialization(self): - btn = LinkButton(url="https://example.com/docs", label="Docs", id_="open-docs") + btn = LinkButton(url="https://example.com/docs", label="Docs", id="open-docs") round_tripped = json.loads(json.dumps(btn)) assert round_tripped["id"] == "open-docs" assert round_tripped["type"] == "link-button"