fix(feishu): route markdown tables through post+tag:md instead of force-text - #53355
fix(feishu): route markdown tables through post+tag:md instead of force-text#53355YzFrocket wants to merge 1 commit into
Conversation
…ce-text Feishu's post-type 'md' element renders the full GFM surface today, including tables. The historic _MARKDOWN_TABLE_RE workaround in _build_outbound_payload predates that and downgrades the *entire* message to msg_type: text whenever a table is detected — which strips formatting from every other markdown element in the same message (headings, bold, code blocks, lists, quotes), making mixed-content replies arrive as raw markdown source on the client. Send tables through post + tag: md like every other markdown shape. The existing call-site already handles a rejected post payload via _POST_CONTENT_INVALID_RE and falls back to plain text, so we do not need a pre-emptive force-text branch. Tests: - New: test_outbound_payload_routes_markdown_tables_to_post_md pins the regression — a message containing a table plus other markdown elements is emitted as a single post+md payload, not downgraded to text. - New: test_outbound_payload_keeps_plain_text_for_non_markdown guards the second branch so we don't widen the post path to plain-text content for no reason. - All 207 tests in tests/gateway/test_feishu.py pass. References: NousResearch#27529 (proposed this exact fix), NousResearch#9549 (original bug report on table rendering), NousResearch#26658 (alternative proposal to remove the detector entirely).
Related/competing: this is the latest entrant in the long-standing Feishu markdown-table rendering cluster — same post+tag:md mechanism as open #29552 and #33800 (canonical #26108 is closed). Note this PR targets the relocated adapter at |
teknium1
left a comment
There was a problem hiding this comment.
Thanks for the focused Feishu fix. The functional direction is sound: current main still returns plain text for matching tables at plugins/platforms/feishu/adapter.py:4528, while Feishu’s current message-content documentation recommends post + md for GFM tables.
Problems
- The new detector comment says it is retained for logging/metrics, but
_MARKDOWN_TABLE_REhas no logging or metrics consumer; its remaining use is outbound routing (plugins/platforms/feishu/adapter.py:158-160,4528).
Suggested changes
- Describe the detector as selecting the post+md path for table-bearing content, rather than as a logging/metrics signal.
This is an automated hermes-sweeper review.
| # workaround now strips formatting from every other element in the same | ||
| # message. We keep the detector so we can still observe table content for | ||
| # logging/metrics, but the outbound payload path uses post + tag:md. | ||
| _MARKDOWN_TABLE_RE = re.compile(r"^\|.*\|\n\|[-|: ]+\|", re.MULTILINE) |
There was a problem hiding this comment.
The detector remains required by the routing condition below, but repository-wide search finds no logging or metrics consumer for it. Please remove that rationale and describe it as selecting the post+md route for table-bearing content.
Summary
Feishu's post-type
mdelement renders the full GFM surface today, including tables. The historic_MARKDOWN_TABLE_REworkaround in_build_outbound_payloadpredates that and force-downgrades the entire message tomsg_type: textwhenever a GFM table appears. That side-effect strips formatting from every other markdown element in the same message (headings, bold, code blocks, lists, quotes), so mixed-content replies render as raw markdown source on the client.This PR routes table-bearing markdown through
post+tag: mdlike every other markdown shape, removing the force-text branch.Repro before this fix
Send any Feishu message that mixes a markdown table with other markdown:
Result on the Feishu client: the whole message arrives as raw text —
##,**,|, fences all visible as characters. Same content without the table renders correctly as a post.Root cause
plugins/platforms/feishu/adapter.py::FeishuAdapter._build_outbound_payload(line 4377 onmain):The first branch fires whenever the regex matches
^|...|\n|---|...anywhere in the message and demotes the whole reply to plain text — losing all other markdown for the sake of "rescuing" the table.The original justification (Feishu's
mdtag didn't render tables) is no longer accurate: the officialcreate_jsondoc lists tables under thetag: mdexample, and empirical testing on current Feishu clients (verified in the linked issue #27529) confirms tables render correctly inside post + md.Fix
Send tables through
post+tag: mdlike every other markdown shape:A pre-emptive text fallback is no longer needed: the send loop already handles a rejected post payload via
_POST_CONTENT_INVALID_REand degrades to plain text on both API exception and unsuccessful API response (existing code,_send_text_messagearound line 1801). So if Feishu ever regresses on table rendering inside posts, behaviour reverts to the current safety net automatically — but on success the receiver gets a properly-rendered post for the table and all surrounding markdown.The detector
_MARKDOWN_TABLE_REitself is preserved (some metrics / logging paths may want to know a message contained a table). Only the routing changes. The stale comment block above the regex is updated to record the new contract.Test Plan
test_outbound_payload_routes_markdown_tables_to_post_mdpins the regression — a message containing a table plus heading + bold + trailing prose is emitted as a single post+md payload, not downgraded to text.test_outbound_payload_keeps_plain_text_for_non_markdownguards the second branch so the change doesn't accidentally widen the post path to plain-text content (which would cost a richer-rendering payload for nothing).tests/gateway/test_feishu.pysuite — 207/207 passed.test_send_falls_back_to_text_when_post_payload_is_rejected,test_send_falls_back_to_text_when_post_response_is_unsuccessful,test_edit_message_falls_back_to_text_when_post_update_is_rejected) still pass — confirms the post-rejection safety net is intact for tables now too.References
tag: mdNot closing #27529 / #9549 — leaving it to maintainers' judgement whether this PR addresses them in full.