fix(slack): render_postable uses AST path for PostableMarkdown (closes #81)#82
Merged
Merged
Conversation
…81) SlackFormatConverter.render_postable was calling _markdown_to_mrkdwn (a simple regex) for PostableMarkdown and {"markdown": ...} dict inputs. The upstream TS SDK routes these through fromAst(parseMarkdown(text)), which correctly handles all markdown constructs — including links — via AST node handlers. The regex had two failure modes the AST path avoids: (a) its link pattern [^)]+ stops at the first ')' so URLs with parentheses were truncated, and (b) it had no equivalent of the link-node branch in _node_to_mrkdwn, making edge-case formatting harder to extend safely. Change both markdown branches in render_postable to call from_markdown instead. str and raw branches are unaffected — they correctly stay on _convert_mentions_to_slack since those inputs are already mrkdwn. Adds TestRenderPostable to test_slack_format.py covering the previously untested render_postable(PostableMarkdown(...)) code path, including link conversion, bold, mixed formatting, query-string URLs, and the str/raw passthrough cases. Closes #81 https://claude.ai/code/session_01XZeM3apTXAHZmjndAqeKPw
|
Caution Review failedPull request was closed or merged during review 📝 WalkthroughWalkthrough
ChangesSlack Message Format Routing
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related issues
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. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Tip 💬 Introducing Slack Agent: The best way for teams to turn conversations into code.Slack Agent is built on CodeRabbit's deep understanding of your code, so your team can collaborate across the entire SDLC without losing context.
Built for teams:
One agent for your entire SDLC. Right inside Slack. 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}
Contributor
There was a problem hiding this comment.
Code Review
This pull request transitions the SlackFormatConverter to use AST-based markdown conversion via from_markdown to ensure consistency with the TypeScript SDK, addressing issue #81. It also adds a comprehensive set of regression tests for the render_postable method. Feedback suggests further improving render_postable by adding support for card types and refining the fallback logic to prevent silent failures when handling unexpected message formats.
Comment on lines
84
to
95
| if "raw" in message: | ||
| return self._convert_mentions_to_slack(message["raw"]) | ||
| if "markdown" in message: | ||
| return self._markdown_to_mrkdwn(message["markdown"]) | ||
| return self.from_markdown(message["markdown"]) | ||
| if "ast" in message: | ||
| return self.from_ast(message["ast"]) | ||
| # Dataclass-style objects | ||
| if hasattr(message, "markdown"): | ||
| return self._markdown_to_mrkdwn(message.markdown) | ||
| return self.from_markdown(message.markdown) | ||
| if hasattr(message, "ast"): | ||
| return self.from_ast(message.ast) | ||
| return "" |
Contributor
There was a problem hiding this comment.
The render_postable implementation in SlackFormatConverter is missing support for PostableCard and CardElement types, which are handled in the base class. Additionally, the fallback behavior returns an empty string instead of str(message), which can lead to silent failures for unexpected message shapes.
Consider aligning this implementation with BaseFormatConverter.render_postable to ensure parity and robustness. Also, when checking for the fallback value, use is not None to avoid silently ignoring falsy but valid values like 0.
Suggested change
| if "raw" in message: | |
| return self._convert_mentions_to_slack(message["raw"]) | |
| if "markdown" in message: | |
| return self._markdown_to_mrkdwn(message["markdown"]) | |
| return self.from_markdown(message["markdown"]) | |
| if "ast" in message: | |
| return self.from_ast(message["ast"]) | |
| # Dataclass-style objects | |
| if hasattr(message, "markdown"): | |
| return self._markdown_to_mrkdwn(message.markdown) | |
| return self.from_markdown(message.markdown) | |
| if hasattr(message, "ast"): | |
| return self.from_ast(message.ast) | |
| return "" | |
| if "raw" in message: | |
| return self._convert_mentions_to_slack(message["raw"]) | |
| if "markdown" in message: | |
| return self.from_markdown(message["markdown"]) | |
| if "ast" in message: | |
| return self.from_ast(message["ast"]) | |
| if "card" in message or message.get("type") == "card": | |
| from chat_sdk.cards import card_to_fallback_text | |
| return card_to_fallback_text(message.get("card") or message) | |
| # Dataclass-style objects | |
| if hasattr(message, "markdown"): | |
| return self.from_markdown(message.markdown) | |
| if hasattr(message, "ast"): | |
| return self.from_ast(message.ast) | |
| if hasattr(message, "card"): | |
| from chat_sdk.cards import card_to_fallback_text | |
| return card_to_fallback_text(message.card) | |
| return str(message) if message is not None else "" |
References
- When checking for optional values that can be falsy but valid (e.g., 0, empty string, empty list), use
is not Noneinstead of a truthiness check to avoid silently ignoring them.
Two pre-existing gaps in SlackFormatConverter.render_postable flagged in review: 1. Card types (PostableCard / CardElement dict) were unhandled; the override returned "" instead of routing through card_to_fallback_text like the base class does. 2. The fallback for unknown message shapes was "" (silent failure) rather than str(message), which matches BaseFormatConverter and is safer for unexpected inputs. Mirrors the base class structure for both cases. No new tests needed: these branches are identical to the base class and covered by its suite. https://claude.ai/code/session_01XZeM3apTXAHZmjndAqeKPw
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (2)
tests/test_slack_format.py (1)
54-107: ⚡ Quick winNo test coverage for the new card and
ast-dict branches added inrender_postable.
TestRenderPostableguards the"markdown"/PostableMarkdownfix well, but the card handling code added in the same PR (lines 91–111 offormat_converter.py) is entirely uncovered:
{"card": <card_payload>}→card_to_fallback_text(message["card"]){"type": "card", ...}+is_card_element→card_to_fallback_text(message)- Object with
.cardattribute →card_to_fallback_text(message.card){"ast": <root>}dict →from_ast(message["ast"])Any regression in those branches would go undetected. Adding a test per branch (even mocking
card_to_fallback_text) costs very little.🤖 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 54 - 107, Test coverage is missing for the new card and ast-dict branches in render_postable; add unit tests in TestRenderPostable that exercise: 1) dict message with "card" key -> ensure render_postable calls card_to_fallback_text(message["card"]) (mock card_to_fallback_text); 2) dict message with "type":"card" and where is_card_element(message) is true -> mock card_to_fallback_text(message) and assert output; 3) object with a .card attribute -> provide a simple object with .card and mock card_to_fallback_text(message.card); and 4) dict with "ast" key -> mock from_ast(message["ast"]) and assert render_postable returns the from_ast result; use the existing TestRenderPostable setup and SlackFormatConverter.render_postable to locate code paths.src/chat_sdk/adapters/slack/format_converter.py (1)
205-221: ⚡ Quick winRemove
_markdown_to_mrkdwn— it is unreachable dead code.This private method has no call sites in the codebase. The test suite confirms that
render_postableshould use the AST-basedfrom_markdownconversion path, not the regex-based helper.🤖 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/format_converter.py` around lines 205 - 221, Remove the unreachable regex-based helper _markdown_to_mrkdwn from the Slack format converter: delete the private method _markdown_to_mrkdwn (and any purely internal references if present) so the module relies solely on the AST-based from_markdown path; ensure no external code references this function remain and run tests to confirm nothing else imports or uses _markdown_to_mrkdwn.
🤖 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.
Nitpick comments:
In `@src/chat_sdk/adapters/slack/format_converter.py`:
- Around line 205-221: Remove the unreachable regex-based helper
_markdown_to_mrkdwn from the Slack format converter: delete the private method
_markdown_to_mrkdwn (and any purely internal references if present) so the
module relies solely on the AST-based from_markdown path; ensure no external
code references this function remain and run tests to confirm nothing else
imports or uses _markdown_to_mrkdwn.
In `@tests/test_slack_format.py`:
- Around line 54-107: Test coverage is missing for the new card and ast-dict
branches in render_postable; add unit tests in TestRenderPostable that exercise:
1) dict message with "card" key -> ensure render_postable calls
card_to_fallback_text(message["card"]) (mock card_to_fallback_text); 2) dict
message with "type":"card" and where is_card_element(message) is true -> mock
card_to_fallback_text(message) and assert output; 3) object with a .card
attribute -> provide a simple object with .card and mock
card_to_fallback_text(message.card); and 4) dict with "ast" key -> mock
from_ast(message["ast"]) and assert render_postable returns the from_ast result;
use the existing TestRenderPostable setup and
SlackFormatConverter.render_postable to locate code paths.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 3000bf21-7b1b-49b3-b13c-8d85ed10cac5
📒 Files selected for processing (2)
src/chat_sdk/adapters/slack/format_converter.pytests/test_slack_format.py
…nch tests
Remove _markdown_to_mrkdwn — a regex-based private method with no call sites
after render_postable was switched to the AST path. Deleting it restores
structural parity with the TS SDK (which has no equivalent method). Also adds
test coverage for the card dict branch, CardElement-style dict branch, .card
attribute branch, and the {"ast": ...} dict branch in render_postable.
https://claude.ai/code/session_01XZeM3apTXAHZmjndAqeKPw
…ranches
Add 19 new tests covering previously untested paths:
- render_postable: {"raw": ...} dict, CardElement dict, .ast attribute,
arbitrary object fallback, multiple simultaneous @mentions
- _node_to_mrkdwn: heading (h1/h2 → bold), blockquote, thematic break,
image with alt, image without alt
- extract_plain_text: strikethrough, bare URL, channel mention with name,
bare channel, named user mention
- to_blocks_with_table: non-dict AST returns None, standalone table emits
no extra section blocks, column alignment produces column_settings
https://claude.ai/code/session_01XZeM3apTXAHZmjndAqeKPw
patrick-chinchill
pushed a commit
that referenced
this pull request
May 7, 2026
Per gemini-code-assist review on PR #83. Without the repo prefix, GitHub auto-links the upstream PR numbers to local PRs in chat-sdk-python, which collides with the local refs (#64, #66, #67, #74, #82) elsewhere in the file. Use vercel/chat#NNN so the upstream refs link correctly. https://claude.ai/code/session_01FyMxQn2BEAzmwKS1GZczKj
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
SlackFormatConverter.render_postablewas routingPostableMarkdownand{"markdown": ...}dict inputs through a private regex helper (_markdown_to_mrkdwn) instead of the AST-basedfrom_markdownpathfromAst(parseMarkdown(text))— this was an undocumented divergence<url|textoutputmarkdownbranches inrender_postablenow callfrom_markdown;strandrawbranches are unaffectedRoot cause
Additional changes in this PR
_markdown_to_mrkdwn— the method had no call sites after the fix; the TS SDK has no equivalent. Removes an undocumented divergence.render_postable—{"card": ...}dict,{"type": "card", ...}CardElement dict,{"ast": ...}dict,.card/.astattribute branches, andstr(message)fallback for unrecognized types.Connection to chinchill-api
This is the SDK-side root cause of the
PostableMarkdownstreaming round-trip bug that causedchinchill-apito abandonPostableMarkdownin favour ofPostableRaw+ a local_SLACK_MARKDOWN_LINK_REregex workaround (chinchill-api#1002, chinchill-api#948). Fixingrender_postablehere makes the SDK correct end-to-end for that use case.Test plan
TestRenderPostable(7 tests): link conversion, dict markdown, bold, mixed, query-string URL, str passthrough, PostableRaw passthroughTestRenderPostableAdditionalBranches(3 tests):{"raw": ...}dict, CardElement dict,.astattribute, arbitrary object fallback, multiple simultaneous mentionsTestRenderPostableRemainingBranches(5 tests): same coverage, integrated into the existing test classTestNodeRendering(6 tests): heading → bold, blockquote, thematic break, image with/without altTestExtractPlainTextAdditional(5 tests): strikethrough, bare URL, channel mentionsTestToBlocksWithTableAdditional(3 tests): non-dict AST, standalone table, column alignmenttest_slack_format.pypass; CI green on 3.10 / 3.11 / 3.12 / 3.13Release
Bumps to
0.4.26.3(Python-only patch; no upstream version change). Changelog updated.https://claude.ai/code/session_01XZeM3apTXAHZmjndAqeKPw
Summary by CodeRabbit
New Features
Bug Fixes
Tests