Skip to content

fix(feishu): render markdown tables as wide-screen interactive cards - #68084

Closed
KnightWorld wants to merge 2 commits into
NousResearch:mainfrom
KnightWorld:fix/feishu-markdown-table-card
Closed

fix(feishu): render markdown tables as wide-screen interactive cards#68084
KnightWorld wants to merge 2 commits into
NousResearch:mainfrom
KnightWorld:fix/feishu-markdown-table-card

Conversation

@KnightWorld

Copy link
Copy Markdown

Summary

Feishu's post-type md elements do not render markdown tables with sane column widths in the narrow message column, and the previous fallback (sending raw table text) produced cramped / effectively blank output on the client.

This ports the @larksuite/openclaw-lark buildMarkdownCard approach so tables render correctly in Feishu.

What changed

  • Tables → wide-screen interactive card: when a message contains a markdown table, send an interactive card (schema: "2.0", wide_screen_mode: true) whose single markdown element holds the entire reply. A single element + <br> markers around the table is what lets Feishu give the grid the full card width (splitting the table into separate elements breaks the wide-screen layout).
  • _optimize_markdown_style(text, card_version=2): ported from upstream markdown-style.js. It parks fenced code blocks behind placeholders, downgrades headings (H1→H4, H2–H6→H5), inserts <br> markers around tables/headings, re-injects code blocks with <br> padding, and collapses excess blank lines.
  • _build_markdown_card_payload(content): builds the card payload (mirrors buildMarkdownCard).
  • Non-table markdown still uses the post path, now with _optimize_markdown_style(card_version=1) so headings/whitespace are normalized.
  • _strip_invalid_image_keys(text): drops ![alt](value) references where value is not a Feishu img_xxx key, preventing CardKit error 200570 from stray URLs/paths.

Verification

  • Verified via a live Feishu send: markdown tables now render with correct column widths, matching OpenClaw's output.
  • _build_outbound_payload routing: table → interactive card; other markdown → post; plain text → text.

Files

  • plugins/platforms/feishu/adapter.py

🤖 Generated with WorkBuddy

Feishu post-type 'md' elements do not render markdown tables with sane
column widths in the narrow message column, and sending raw table text
previously produced cramped/blank output.

Port the @larksuite/openclaw-lark buildMarkdownCard approach:
- When a message contains a markdown table, send a wide-screen
  interactive card whose single 'markdown' element holds the whole reply.
- _optimize_markdown_style (card_version=2) inserts <br> markers around
  tables so the renderer treats them as separate visual units and gives
  them the full card width.
- Non-table markdown still uses the post path with card_version=1 style
  optimization.
- _strip_invalid_image_keys drops non-Feishu image references to avoid
  CardKit error 200570.

Verified via live Feishu send: tables now render with correct column
widths, matching OpenClaw output.
@alt-glitch alt-glitch added type/bug Something isn't working comp/plugins Plugin system and bundled plugins platform/feishu Feishu / Lark adapter P3 Low — cosmetic, nice to have sweeper:risk-message-delivery Sweeper risk: may drop, duplicate, misroute, or suppress messages duplicate This issue or pull request already exists labels Jul 20, 2026
@alt-glitch

Copy link
Copy Markdown
Collaborator

This was generated by AI during triage.

Duplicate of #29630: both route Feishu Markdown tables to a Schema 2.0 wide-screen interactive card using a Markdown element. #29630 is the earlier open implementation.

@KnightWorld

Copy link
Copy Markdown
Author

Thanks for the triage note. The approach here does match #29630 (both route Feishu markdown tables to a Schema 2.0 wide-screen interactive card using a single markdown element), but I believe this PR is the actionable version of that idea rather than a true duplicate to be closed:

On the 230099 CardKit failure that closed the related #27990: that PR used a different card structure. This implementation follows the same single-markdown-element + <br> pattern that OpenClaw's @larksuite/openclaw-lark buildMarkdownCard uses, and is confirmed rendering correctly in production.

Happy to add tests or extend the interactive → post → text fallback to the edit/streaming paths if maintainers want that before merge.

@Bryntly Bryntly left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The Feishu markdown optimizations look great for fixing the table rendering! However, there are two issues related to how fenced code blocks are handled during the optimization pipeline:

  1. Mutating fenced code blocks:
    Code blocks are extracted and replaced with placeholders to prevent mutation, but they are re-injected before the final steps:
    # 3f. Re-inject code blocks with ``<br>`` padding
    ...
    # 4. Collapse 3+ newlines to 2
    r = re.sub(r"\n{3,}", "\n\n", r)
    r = _strip_invalid_image_keys(r)

This means that any 3+ consecutive newlines inside a fenced code block will be incorrectly collapsed, and any markdown image syntax inside a code block (e.g. in a comment or string literal) will be stripped out!
Fix: Move the code block re-injection (the if card_version >= 2 and else blocks) to the very end of the function, after re.sub and _strip_invalid_image_keys(r).

  1. Double newline prefix on re-injection:
    In _park_fence, match.group(0) is appended to code_blocks. This includes the prefix (the leading \n or ^). The replacement placeholder also includes the prefix: return f"{prefix}{_FENCE_PLACEHOLDER_PREFIX}...".
    When the code blocks are re-injected, r.replace(placeholder, code_blocks[i]) replaces the placeholder (which is preceded by prefix in r) with code_blocks[i] (which also starts with prefix), causing the leading newline to be duplicated.
    Fix: Store the code block without the prefix:
    def _park_fence(match):
        idx = len(code_blocks)
        prefix = match.group(1) or ""
        code_blocks.append(match.group(0)[len(prefix):])
        return f"{prefix}{_FENCE_PLACEHOLDER_PREFIX}{idx}__{idx}"

Please apply these fixes to ensure code blocks remain untouched by the optimizer!

@Bryntly Bryntly left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have reviewed the code. As pointed out in my previous comment, there are critical bugs in the markdown optimization pipeline regarding how fenced code blocks are re-injected. Specifically:

  1. Code blocks are re-injected before collapsing newlines and stripping invalid image keys, which mutates the contents of the fenced code blocks unexpectedly. The re-injection must happen at the very end of the _optimize_markdown_style function.
  2. _park_fence includes the prefix in both the stored block and the placeholder, which leads to a duplicated prefix upon re-injection.

Please fix these issues to prevent data corruption within code blocks.

@Bryntly Bryntly left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have reviewed the changes for security issues.

Security Vulnerability: Filter Evasion / DoS in _strip_invalid_image_keys

The regex used to strip invalid image keys is vulnerable to evasion. The current pattern:

_IMAGE_RE = re.compile(r"!\[([^\]]*)\]\(([^)\s]+)\)")

It explicitly denies spaces inside the URL parentheses [^)\s]+. However, valid Markdown allows titles in images, separated by a space (e.g., ![alt](http://evil.com "title")).

Because of the space before "title", this payload will fail to match _IMAGE_RE. As a result, _strip_invalid_image_keys will leave the stray URL in the text instead of stripping it.
As stated in the comments, allowing a stray URL to slip through causes a CardKit error 200570. By sending a specially crafted markdown image, an attacker or user can bypass the safety net, trigger this CardKit error, and potentially cause a Denial of Service (message delivery failure) for the Feishu bot.

Recommendation:
Update the regex to capture the entire content inside the parentheses, and then strip it before checking for the img_ prefix:

_IMAGE_RE = re.compile(r"!\[([^\]]*)\]\(([^)]+)\)")

And in the lambda:

lambda m: m.group(0) if m.group(2).strip().startswith("img_") else ""

Apart from this, the JSON payload construction is secure against injection and there are no catastrophic backtracking (ReDoS) vulnerabilities in the markdown regexes.

@Bryntly Bryntly left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have reviewed the code comments and docstrings in this PR.

  • The docstring for _build_markdown_card_payload clearly explains the rationale for using a single markdown element and <br> markers for wide-screen mode.
  • The block comment for Markdown style optimization for Feishu post rendering correctly sets the context for the ported logic.
  • The docstring for _optimize_markdown_style accurately details the 4-step pipeline implemented in the function.
  • The inline comments in _build_outbound_payload effectively explain the routing logic based on markdown tables.

Overall, the documentation is accurate, clear, and well-maintained. Good job on preserving the context from @larksuite/openclaw-lark.

@Bryntly

Bryntly commented Jul 20, 2026

Copy link
Copy Markdown

HUMAN INTERVENTION REQUIRED

The AI Fixer agent successfully applied the necessary fixes locally for the code block re-injection mutation, prefix duplication, and filter evasion vulnerability in _IMAGE_RE.

However, it was unable to push these fixes to the PR branch (KnightWorld/hermes-agent:fix/feishu-markdown-table-card) due to a 403 Permission Denied error (likely because "Allow edits from maintainers" is disabled on this fork, or the agent lacks permissions).

@KnightWorld, please fetch the fixes from the BRYNTLY-ORG/HERMES repository (branch: fix/feishu-markdown-table-card) or enable "Allow edits from maintainers" so we can push the changes and complete the review/merge process.

@alt-glitch alt-glitch removed the duplicate This issue or pull request already exists label Jul 20, 2026
@alt-glitch

Copy link
Copy Markdown
Collaborator

This was generated by AI during triage.

Related to #29630 and #12114: #29630 targets the retired gateway/platforms/feishu.py path, while this PR ports the same broad card-rendering goal to the current plugins/platforms/feishu/adapter.py path. It is therefore a related current-path implementation, not a duplicate; maintainers should choose or consolidate the approach.

…x dup, image regex)

- Re-inject parked code blocks LAST so the optimize steps (newline
  collapsing and non-Feishu image stripping) never mutate code contents.
- Store fence without its leading prefix to avoid a duplicated newline on
  re-injection.
- Tighten _IMAGE_RE to [^)]+ and require an img_ prefix so titles/URLs
  (e.g. ![alt](http://evil.com "title")) cannot evade the CardKit 200570
  guard.

Addresses Bryntly review on PR NousResearch#68084.
@KnightWorld

Copy link
Copy Markdown
Author

Thanks for the thorough review. I applied the three fixes on my own fork rather than pulling from the BRYNTLY-ORG/HERMES branch — that's a third-party repo I don't control, so I'd rather keep the change under my own authorship.

All three are addressed in commit 3c8ada3:

  • Parked code blocks are now re-injected last, after newline collapsing and image-key stripping, so their contents are never mutated by those steps.
  • The fence parker no longer stores the leading newline/prefix, so re-injection doesn't duplicate it.
  • _IMAGE_RE now matches ![alt](value) with spaces inside value, and only keeps entries whose key starts with img_. That closes the CardKit 200570 filter-evasion gap you flagged.

Verified via a live Feishu send: tables render with correct column widths. Happy to add unit tests or extend the fallback chain to the edit/streaming paths if maintainers want that before merge.

@teknium1

Copy link
Copy Markdown
Contributor

Thanks for the current-path port and for addressing the code-block and image-key review feedback.

Automated hermes-sweeper review found that the underlying table-rendering issue is already fixed on main:

  • plugins/platforms/feishu/adapter.py:4590-4604 now routes recognized pipe tables through Feishu's native post/md path rather than plain text.
  • tests/gateway/test_feishu_table_markdown.py:66-87 verifies a table uses post and preserves its table content.
  • The behavior shipped through merged PR fix(feishu): render markdown tables via post+md, remove force-text downgrade #68121 (a660630986, release v2026.7.20), which also retains the existing post-to-text rejection fallback.

Closing as implemented on main.

@teknium1 teknium1 closed this Jul 30, 2026
@teknium1 teknium1 added the sweeper:implemented-on-main Sweeper: behavior already present on current main label Jul 30, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

comp/plugins Plugin system and bundled plugins P3 Low — cosmetic, nice to have platform/feishu Feishu / Lark adapter sweeper:implemented-on-main Sweeper: behavior already present on current main sweeper:risk-message-delivery Sweeper risk: may drop, duplicate, misroute, or suppress messages type/bug Something isn't working

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants