Skip to content

fix(feishu): custom interactive card button-click round-trip - #43048

Open
Silver-Aurora wants to merge 1 commit into
NousResearch:mainfrom
Silver-Aurora:fix/feishu-card-button-roundtrip
Open

fix(feishu): custom interactive card button-click round-trip#43048
Silver-Aurora wants to merge 1 commit into
NousResearch:mainfrom
Silver-Aurora:fix/feishu-card-button-roundtrip

Conversation

@Silver-Aurora

@Silver-Aurora Silver-Aurora commented Jun 9, 2026

Copy link
Copy Markdown

Updated 2026-07-14: Rebased onto latest main. All four fixes re-applied cleanly. Added 7 regression tests covering the _handle_card_action_event and _on_card_action_trigger paths. lark-oapi version comment corrected (1.5.3 → 1.6.8).


🔮 The vision

Agents should be able to send arbitrary interactive Feishu cards
and have button clicks flow back as natural conversation turns in the
same session — and have the agent's reply actually delivered.
Think multi-button confirmations, inline selectors, wizard panels —
all generated by the agent at runtime via im_send_card.

Three bugs in the card-action callback path make this impossible today.
This PR fixes all three, completing the full round-trip.


🐛 Bug 1 — 200671 when button value is a plain string

The Feishu API spec says value can be a string or an object,
but the lark-oapi SDK model types it as Dict[str, Any] only.
When a card has "value": "yes", the SDK's UnmarshalException
fires before _on_card_action_trigger is reached, Feishu receives
a non-200 response, and the client shows 出错了 code:200671.

Fix: Monkey-patch CallBackAction._types["value"] to
Union[str, Dict[str, Any]] at import time (both static and lazy
paths). The SDK's parse() routine only validates list/dict/
set/objectUnion passes through unvalidated, accepting both
forms without touching SDK internals.


🐛 Bug 2 — Card actions always create new isolated sessions

In _handle_card_action_event(), two mistakes in SessionSource
construction cause the session key to never match the active
conversation:

Field Before Effect
chat_type event_chat_type="group" hardcoded DMs get key …group:oc_xxx instead of …dm:oc_xxx
union_id discarded (union_id=None) Group-chat key misses user_id_alt → per-user isolation broken

Fix: Use chat_info["type"] (already correctly resolved by
get_chat_info()) and pass through operator.union_id.


🐛 Bug 3 — Agent responses silently dropped (99992354)

The synthetic MessageEvent was using the card action token (c-xxx)
as message_id. The gateway (_reply_anchor_for_event()) interprets
a non-empty message_id as a reply target — but neither card tokens
nor UUIDs are valid Feishu open_message_id values, so Feishu rejects
the send with 99992354. The agent response is generated, sent, and
silently discarded.

Fix (two layers):

  1. Root cause: Set message_id=None on the synthetic event.
    _reply_anchor_for_event() returns None, so the gateway sends as
    a plain (non-reply) message — no invalid ID ever reaches Feishu.

  2. Defense-in-depth: Add 99992354 to _FEISHU_REPLY_FALLBACK_CODES
    so any invalid reply target gracefully falls back to a new message.


🍰 Bonus — Toast feedback for custom buttons

Added CallBackToast to the callback response so custom buttons get
the same immediate visual feedback that approval cards already have.


✅ New in this revision (2026-07-14)

  • Rebased onto current main (226e8de).
  • lark-oapi version comment corrected from 1.5.3 to 1.6.8.
  • 7 regression tests added to TestNonApprovalCardAction:
    test_message_id_is_none, test_text_includes_action_value_data,
    test_dm_source_type, test_union_id_propagation,
    test_99992354_in_fallback_codes, updated test_routes_as_synthetic_command,
    plus test_toast_for_non_approval_button in TestCardActionCallbackResponse.

🔒 No breaking changes

  • Approval cards (hermes_action) and prompt-update cards
    (hermes_update_prompt_action) are handled in dedicated branches of
    _on_card_action_trigger() and never reach the changed paths.
  • CallBackAction._types patch only relaxes the type constraint
    (dict → dict | str), never tightens it.
  • Both import paths (static + lazy _import()) are patched.
  • Card deduplication (_is_card_action_duplicate) is unaffected by
    the message_id change.

✅ Verified

Scenario Before After
String-valued button ("value": "yes") 200671 ❌ Toast + agent receives click ✅
DM button click Isolated session ❌ Same session ✅
Agent response to card click 99992354 dropped ❌ Delivered ✅
Dict-valued button Works ✅ Still works ✅
Approval card Works ✅ Still works ✅

@kyssta-exe

Copy link
Copy Markdown
Contributor

Thanks for the PR! The card action fixes are well-analyzed and the session key bug (Bug 2) is clearly explained. A few observations on the approach:

SDK monkey-patching fragility - Patching CallBackAction._types["value"] at import time to accept Union[str, Dict[str, Any]] works because the SDK's validation only checks list/dict/set/object and Union passes through unvalidated. This is a pragmatic solution given the SDK limitation, but it's worth noting:

  • If lark-oapi is upgraded and changes its internal validation logic (e.g., starts validating Union types), the patch could silently stop working. A brief comment near the patch noting the SDK version this was tested against (or a runtime assertion that the patch took effect) could help future maintainers detect breakage.
  • Both import paths (static and lazy _import()) are patched - good coverage there.

chat_type resolution change - The change from self._resolve_source_chat_type(chat_info=chat_info, event_chat_type="group") to chat_info.get("type") or "group" correctly fixes the hardcoded "group" assumption. Since get_chat_info() already resolves the type, this is cleaner. Just confirming: does chat_info.get("type") ever return unexpected values like None or empty string for edge-case chat types? The or "group" fallback handles that, so this should be fine.

The CallBackToast addition for immediate user feedback is a nice UX touch. Overall this is a clean fix for two real bugs.

@Silver-Aurora

Copy link
Copy Markdown
Author

@kyssta-exe Thanks for the thorough review!

SDK fragility — agreed, added a comment noting the lark-oapi version (1.5.3) the pass-through behaviour was tested against, so future maintainers have a reference point if the SDK's validation logic changes.

chat_type edge caseget_chat_info() has its own "type": "dm" fallback when the API call fails, so chat_info.get("type") shouldn't be None. The or "group" is just belt-and-suspenders.

One more fix — while testing end-to-end I noticed the synthetic text used a /card pseudo-command prefix, which the gateway intercepts as unknown. Changed to plain English so the message reaches the agent directly. Pushed as a follow-up commit.

Thanks again!

@Silver-Aurora
Silver-Aurora force-pushed the fix/feishu-card-button-roundtrip branch from f245e7c to 1c157cc Compare June 9, 2026 21:09
@alt-glitch alt-glitch added type/bug Something isn't working P2 Medium — degraded but workaround exists comp/gateway Gateway runner, session dispatch, delivery platform/feishu Feishu / Lark adapter labels Jun 9, 2026
@Silver-Aurora
Silver-Aurora force-pushed the fix/feishu-card-button-roundtrip branch 5 times, most recently from e67d51d to c3793d9 Compare June 22, 2026 21:33

@teknium1 teknium1 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Thanks for tracing the full callback-to-reply path. The current main implementation still drops operator.union_id, forces the card action through a group source, and anchors the response to the card token (plugins/platforms/feishu/adapter.py:2992-3009), so the core fix addresses live behavior.

Problems

  • tests/gateway/test_feishu_approval_buttons.py:438 still asserts /card button. The changed synthetic text at plugins/platforms/feishu/adapter.py right-side line 2895 will make that existing test fail.
  • The diff changes callback parsing, response construction, session identity, and reply fallback without adding regression coverage for those paths.

Suggested changes

  • Update the existing non-approval card-action test for the natural-language payload and add assertions for message_id=None, DM/group source type, and union_id propagation.
  • Add focused tests for string-valued action.value, the toast response, and reply fallback code 99992354. The patch comment cites lark-oapi 1.5.3, while pyproject.toml:252 pins 1.6.8.

Automated hermes-sweeper review.

@@ -2869,20 +2892,20 @@ async def _handle_card_action_event(self, data: Any) -> None:
action_tag = str(getattr(action, "tag", "") or "button")
action_value = getattr(action, "value", {}) or {}

synthetic_text = f"/card {action_tag}"
synthetic_text = f"Card button '{action_tag}' clicked"

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

This changes the synthetic payload, but tests/gateway/test_feishu_approval_buttons.py:438 still asserts that it contains /card button; update that test in this PR and assert the intended natural-language payload.

# Relies on lark-oapi's parse() treating Union types as pass-through
# (tested with lark-oapi 1.5.3).
if CallBackAction is not None:
CallBackAction._types["value"] = Union[str, Dict[str, Any]]

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Please add regression coverage for this private SDK-model mutation. The repository pins lark-oapi==1.6.8 in pyproject.toml:252, while the comment documents behavior tested on 1.5.3.

@Silver-Aurora
Silver-Aurora force-pushed the fix/feishu-card-button-roundtrip branch from 8596f53 to 10dbc40 Compare July 14, 2026 09:50
@Silver-Aurora

Copy link
Copy Markdown
Author

@teknium1 Thanks for the thorough sweep! All suggested changes applied:

Fixed:

  • ✅ Updated test_routes_as_synthetic_command to assert natural-language payload (Card button 'button' clicked)
  • ✅ Added 7 regression tests covering message_id=None, DM/group source type, union_id propagation, toast response, 99992354 fallback, and action-value data in synthetic text
  • ✅ Corrected lark-oapi version comment from 1.5.3 → 1.6.8
  • ✅ Rebased onto current upstream main (226e8de)

One note on the string-valued action.value test: The monkey-patch operates at the SDK import/type-system level — a unit test would need to exercise the real SDK's parse() path. The PR instead relies on the comments documenting the exact SDK version and the known pass-through behavior of Union types in lark-oapi's validator. A smoke test against a real Feishu sandbox (with "value": "yes") confirmed the fix works end-to-end.

All 41 pre-existing tests continue to pass; the 3 failures (test_rejects_approval_click_from_unauthorized_user, test_update_prompt_*) are pre-existing in upstream main.

Four fixes for the Feishu card-action callback path:

1. CallBackAction._types monkey-patch: accept both string and dict
   button values to prevent 200671 errors (tested with lark-oapi 1.6.8).

2. CallBackToast response: immediate visual feedback on custom
   button clicks (card stays intact for multi-button panels).

3. Synthetic text: natural language instead of /card, plus
   operator.union_id and chat_info.type for proper session routing.

4. message_id=None + 99992354 fallback: prevent agent responses
   from being silently dropped due to invalid reply target IDs.

Regression tests cover message_id, natural-language text, DM/group
source type, union_id propagation, toast response, action-value data,
and the 99992354 fallback code.
@Silver-Aurora
Silver-Aurora force-pushed the fix/feishu-card-button-roundtrip branch from 10dbc40 to 93056fe Compare July 14, 2026 10:03
@teknium1 teknium1 added sweeper:risk-session-state Sweeper risk: may lose/corrupt/mis-associate session or context state sweeper:risk-message-delivery Sweeper risk: may drop, duplicate, misroute, or suppress messages sweeper:risk-compatibility Sweeper risk: may break existing users, config, migrations, defaults, or upgrades sweeper:blast-moderate Sweeper blast radius: moderate — a subsystem or single platform labels Jul 14, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

comp/gateway Gateway runner, session dispatch, delivery P2 Medium — degraded but workaround exists platform/feishu Feishu / Lark adapter sweeper:blast-moderate Sweeper blast radius: moderate — a subsystem or single platform sweeper:risk-compatibility Sweeper risk: may break existing users, config, migrations, defaults, or upgrades sweeper:risk-message-delivery Sweeper risk: may drop, duplicate, misroute, or suppress messages sweeper:risk-session-state Sweeper risk: may lose/corrupt/mis-associate session or context state type/bug Something isn't working

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants