feat(slack): webClientOptions + @mention-in-URL fix + stable link-button action_id (chat@4.31)#155
Conversation
|
Warning Review limit reached
More reviews will be available in 46 minutes and 8 seconds. Learn how PR review limits work. Your organization has used up its prepaid credits, and credit purchases are no longer available. Enable the review add-on in the billing tab to keep reviews running — you're only billed for reviews past your plan's rate limits ($0.25/file). ⌛ How to resolve this issue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based credits. 🚦 How do rate limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan refill rate. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, the refill rate gradually slows as usage increases. The highest same-day bursts are limited more strictly. 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 (13)
📝 WalkthroughWalkthroughThree independent Slack adapter improvements: ChangesSlack Adapter: web_client_options, LinkButton id→action_id,
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 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 introduces support for forwarding custom client options (web_client_options) to slack_sdk clients in the Slack adapter, ensuring proper header isolation across clients. It also adds support for an optional custom id on LinkButton elements, which is mapped to the Slack action_id verbatim (with fallback to the URL-derived action ID). Additionally, it fixes a bug in the Slack format converter where bare @mentions inside URLs were incorrectly rewritten as Slack mentions, corrupting the links. Extensive test coverage has been added for all these features and bug fixes. I have no feedback to provide as there are no review comments.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
![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: 11c8e64e3e
ℹ️ 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".
| # ``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 |
There was a problem hiding this comment.
Because SlackAdapterConfig is a regular dataclass rather than kw_only, inserting this field before the existing app_token/bot_token fields changes the generated positional __init__ signature. Any existing code that passed positional config values, e.g. SlackAdapterConfig(None, "xapp-...", "xoxb-..."), now stores the app token string in web_client_options and shifts the remaining values, which can leave the adapter misconfigured or later fail in _web_client_kwargs when it treats that string as a dict. Appending the new option after the existing fields preserves the public positional ABI.
Useful? React with 👍 / 👎.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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/cards.py`:
- Around line 232-243: The action_id assignment in the SlackLinkButtonElement_
dictionary does not enforce a consistent maximum length limit for both the
explicit button_id and the URL-fallback cases. Currently, only the URL-fallback
branch applies truncation with url[:200], while an explicitly provided button_id
is used unbounded, which can exceed Slack's action_id character limit. Apply the
same truncation limit to both branches by modifying the conditional expression
that assigns action_id to ensure that both button_id (when not None) and the
fallback URL-based id are truncated to the same maximum character length.
In `@tests/test_slack_format.py`:
- Around line 285-294: The test methods
test_email_address_still_preserved_after_fix and
test_mailto_link_still_preserved_after_fix are duplicating assertions that are
already covered by earlier canonical tests. Remove both of these duplicate test
methods entirely, keeping only the original tests that verify email and mailto
link preservation behavior.
🪄 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: 9b6fcc3f-8032-4dbf-bfe4-68de89258880
📒 Files selected for processing (13)
docs/UPSTREAM_SYNC.mdsrc/chat_sdk/adapters/slack/adapter.pysrc/chat_sdk/adapters/slack/blocks/__init__.pysrc/chat_sdk/adapters/slack/blocks/types.pysrc/chat_sdk/adapters/slack/cards.pysrc/chat_sdk/adapters/slack/format_converter.pysrc/chat_sdk/adapters/slack/types.pysrc/chat_sdk/cards.pytests/test_adapter_api_url_config.pytests/test_cards.pytests/test_slack_blocks_primitives.pytests/test_slack_cards.pytests/test_slack_format.py
| # `??` 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": { | ||
| "type": "plain_text", | ||
| "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, |
There was a problem hiding this comment.
Truncate explicit id before assigning Slack action_id.
Line 242 applies truncation only in the URL-fallback branch; a provided id is used unbounded. Very long ids can exceed Slack’s action_id limit and fail at runtime. Please enforce one shared max-length path for both explicit-id and fallback-id generation.
Suggested fix
def _convert_link_button_to_element(button: LinkButtonElement) -> SlackLinkButtonElement_:
@@
- button_id = button.get("id")
+ button_id = button.get("id")
+ action_id = (button_id if button_id is not None else f"link-{url}")[:255]
element: SlackLinkButtonElement_ = {
@@
- "action_id": button_id if button_id is not None else f"link-{url[:200]}",
+ "action_id": action_id,
"url": url,
}🤖 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 `@src/chat_sdk/adapters/slack/cards.py` around lines 232 - 243, The action_id
assignment in the SlackLinkButtonElement_ dictionary does not enforce a
consistent maximum length limit for both the explicit button_id and the
URL-fallback cases. Currently, only the URL-fallback branch applies truncation
with url[:200], while an explicitly provided button_id is used unbounded, which
can exceed Slack's action_id character limit. Apply the same truncation limit to
both branches by modifying the conditional expression that assigns action_id to
ensure that both button_id (when not None) and the fallback URL-based id are
truncated to the same maximum character length.
| 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 <mailto:user@example.com>") == { | ||
| "text": "Email <mailto:user@example.com>" | ||
| } |
There was a problem hiding this comment.
Remove duplicated regression checks for email/mailto behavior.
Line 285 and Line 291 re-test the same assertions already covered in Line 207 and Line 220, so they don’t add new regression signal and increase maintenance overhead. Keep one canonical test per behavior.
As per coding guidelines, "No two tests should verify the same thing. Duplicates inflate counts without catching more bugs."
🤖 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_format.py` around lines 285 - 294, The test methods
test_email_address_still_preserved_after_fix and
test_mailto_link_still_preserved_after_fix are duplicating assertions that are
already covered by earlier canonical tests. Remove both of these duplicate test
methods entirely, keeping only the original tests that verify email and mailto
link preservation behavior.
Source: Coding guidelines
…1657a)
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').
…, 8336a3e)
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.
…, a8bf99a) 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 (?<![<\w]) -> (?<![<\w/]) (kept fixed-width, single char) so a schemeless host path (mastodon.social/@user) is preserved - add URL_REGEX (\bhttps?://[^\s<>]+) 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.
…ching Button/Select 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).
patrick-chinchill
force-pushed
the
feat/4.31-slack-webclient-mention
branch
from
11c8e64 to
0170d85
Compare
* chore(release): cut 0.4.31 — sync to chat@4.31.0 Bumps version 0.4.30 → 0.4.31 and UPSTREAM_PARITY → 4.31.0; re-pins the fidelity check + CI clone to chat@4.31.0 (string-only — the mapped-core test files are byte-identical 4.30→4.31, 732/732 still pass). Wave content (already merged): Linear agent-sessions L1–L5 (#151), Teams SDK-free primitive subpaths, Slack 4.31 (#155), Telegram rich messages, LinkButton stable id, and the opt-in ThinkingChunk stream type (#169). Documents the new chat/adapters static catalog as an intentional non-port (npm-addressed + vendor adapters not shipped here; no consumer need). * docs(upstream-sync): correct adapters-catalog counts (19 tests, ~25 adapters)
1 participant
Wave-A 4.31 parity slice for the Slack adapter — three independent pieces grouped as one PR (one commit each), ported faithfully from
chat@4.31.0.(A) Stable link-button
action_id(171657a)The new core
LinkButtonElement.idis consumed as the Slack link-buttonaction_idwith??semantics: an explicit (even empty-string) id is used verbatim; only a missing/Noneid falls back tolink-{url}.src/chat_sdk/cards.py:LinkButton()factory +LinkButtonElementgain optionalidadapters/slack/cards.py(_convert_link_button_to_element):id ?? f"link-{url[:200]}"adapters/slack/blocks/__init__.py(_link_button_to_element):id ?? f"link-{url}", still_truncate_text(..., LIMITS.action_id)on the id pathadapters/slack/blocks/types.py:SlackLinkButtonElementgainsid: NotRequired[str](B)
web_client_options(8336a3e)SlackAdapterConfig.web_client_options: dict[str, Any] | None, spread into BOTH WebClient construction sites (defaultAsyncWebClient+ per-token syncWebClient).DIVERGENCE: maps to slack_sdk
WebClientkwargs (timeoutin seconds,retry_handlers), not@slack/web-apiaxios options — no 1:1retryConfig/rejectRateLimitedCalls. Documented in the config docstring and adocs/UPSTREAM_SYNC.mdrow.Hazards handled: spread gated on
is not None(an empty{}still spreads); any nestedheadersdict is deep-copied per client (_web_client_kwargs) so cached per-token clients don't share a mutable dict and caller input isn't mutated.(C) @mention-in-URL fix (a8bf99a) — confirmed pre-existing converter bug
A bare
@handleinside a URL (path/query/fragment) must NOT be rewritten into a<@handle>Slack mention. Inadapters/slack/format_converter.py:(?<![<\w])→(?<![<\w/])(kept fixed-width) for the schemeless host caseURL_REGEX+_link_bare_mentions_outside_urls()and call it from BOTH substitution sites (_finalizeand the_node_to_mrkdwntext branch)re.ASCIIso\wmatches JS's ASCII\wexactlyTests
test_cards.py::TestLinkButtonFactory,test_slack_cards.py,test_slack_blocks_primitives.py— custom id verbatim / URL fallback /id==""at both sites + action_id truncationtest_adapter_api_url_config.py::TestSlackWebClientOptions— option reaches default + per-token client, header isolation + no input mutation, empty{}still spreadstest_slack_format.py::TestMentions— 6 upstream cases + adversarial sweep on both string and mrkdwn surfaces. 12 of these FAIL on pre-fix code (verified).Gauntlet (from worktree)
ruff check: passruff format --check: passaudit_test_quality.py: 0 hard failurespyrefly check: 0 errorspytest tests/: 4865 passed, 4 skippedverify_test_fidelity.py --strict(pinnedchat@4.29.0): 732/732, 0 missingSummary by CodeRabbit
New Features
idfield to specify custom action identifiers for link clicksweb_client_optionsconfiguration to forward options to all WebClient instancesBug Fixes
@mentionsinside URLs are now preserved and no longer converted to Slack mention syntaxDocumentation