Skip to content

fix(feishu): include parent media context for replies - #13115

Open
tangyuanjc wants to merge 1 commit into
NousResearch:mainfrom
tangyuanjc:fix/feishu-reply-image-context
Open

fix(feishu): include parent media context for replies#13115
tangyuanjc wants to merge 1 commit into
NousResearch:mainfrom
tangyuanjc:fix/feishu-reply-image-context

Conversation

@tangyuanjc

Copy link
Copy Markdown

Summary

  • Fetch full parent message context for Feishu replies, including downloadable image/file media resources
  • Merge parent media into the inbound MessageEvent so reply-to-image prompts preserve visual context
  • Keep the existing text-only helper compatible via the new context fetcher

Tests

  • /Users/tangyuanjc/.hermes/hermes-agent/venv/bin/python -m pytest tests/gateway/test_feishu.py

Result: 126 passed, 70 warnings (dependency deprecation warnings from lark_oapi/websockets).

@Christianye

Copy link
Copy Markdown

Hi @tangyuanjc — thanks for putting this up. We've been running an
equivalent local patch in a real Lark deployment for several weeks and
can confirm the approach works. Two small refinements worth folding in
before merge, plus one design question.

1. LRU-bound _message_context_cache

_message_text_cache was given an LRU eviction cap in e8cacb57d (PR by
@teknium1) precisely because unbounded Dict[str, ...] caches in
long-lived adapters grew without bound for high-throughput rooms. The new
_message_context_cache here re-introduces the same pattern.

Suggest matching the existing convention — OrderedDict + cap (we use 256
in our local patch; matches the _FEISHU_MESSAGE_TEXT_CACHE_SIZE = 512
order of magnitude), with move_to_end on hit and popitem(last=False)
on overflow:

_FEISHU_MESSAGE_PARENT_CACHE_SIZE = 256

# in __init__
self._message_context_cache: "OrderedDict[str, tuple[Optional[str], List[str], List[str]]]" = OrderedDict()

This also dovetails with @teknium1's 32899279a cache-normalization work.

2. Don't promote inbound_type from TEXT when hydrating parent media

The current diff prepends reply_media_urls onto media_urls on the
event, which is correct, but it's worth being explicit in a comment
that the inbound event's message_type should stay TEXT. Two reasons:

  • The user's actual contribution is a text question — promoting to
    IMAGE/AUDIO misrepresents what the user sent.
  • gateway/run.py media batching keys on media_types (e.g. audio/m4a
    → STT) regardless of message_type, so STT/vision still fire correctly.
  • Keeping message_type=TEXT avoids the inbound being dragged into a
    photo-burst batch when the user happens to be in a chat that's also
    receiving an image burst — we hit this in testing and a comment in the
    source saved us a future regression.

Two-line comment in _process_inbound_message is enough.

3. Test coverage gap (offering to contribute)

The new test_fetch_message_context_downloads_parent_image covers the
image case nicely. We have a 187-line test addition locally that adds:

  • audio/voice parent (msg_type=audio) → audio/m4a MIME + path
  • file parent (msg_type=file) → file path passthrough
  • multiple replies to same parent → only one download (cache hit path)
  • LRU eviction once cap exceeded
  • inbound message_type=TEXT invariant when parent has media
  • existing-media-on-current-event preserved + dedup against parent_media

Happy to PR these on top of yours once this lands, or you're welcome to
cherry-pick — your call. Just want to make sure the regression net is
tight so this doesn't break again.

4. (question) ordering: parent_media first vs current_media first?

Diff prepends parent media ([*reply_media_urls, *media_urls]). Our local
patch did the opposite ([*current, *parent]) on the assumption that
"what the user attached now" is more salient than "what they're
referring to." Either is defensible — just flagging in case there's an
intentional reason for parent-first I'm missing (e.g. ordering matters
for some downstream model adapter we haven't tested against).

Either way, +1 to land this — 6 duplicate PRs deep is not a healthy
state for an issue this clearly defined. Yours is the oldest and the
cleanest split between _fetch_message_text and _fetch_message_context.

@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 isolating the parent-context behavior; the current main path still loses replied-parent media: plugins/platforms/feishu/adapter.py:3259 fetches only reply text, and :3298-3299 forwards only current-message media.

Problems

  • The diff cannot be applied directly: Feishu was moved from gateway/platforms/feishu.py to plugins/platforms/feishu/adapter.py by 5600105478ffde29d7566b45421b100eaa29c4ef; GitHub currently reports this PR as dirty.
  • The added _message_context_cache is unbounded. Current reply-text caching is explicitly LRU-bounded at plugins/platforms/feishu/adapter.py:257,4165-4189 (e8cacb57d531137dec3109617e807b30ff5187c9).
  • A port must retain parent mention normalization: current lookup passes parent.mentions and self._bot_identity() at plugins/platforms/feishu/adapter.py:4181-4186.

Suggested changes

  • Port the context helper and event-media merge into plugins/platforms/feishu/adapter.py, using its _run_blocking path.
  • Use a bounded OrderedDict context cache matching the existing LRU convention.
  • Add audio/file, cache-hit/eviction, current-media preservation, and text-message-type regression tests.

Automated hermes-sweeper review.

self._sent_message_id_order: List[str] = [] # LRU order for _sent_message_ids_to_chat
self._chat_info_cache: Dict[str, Dict[str, Any]] = {}
self._message_text_cache: Dict[str, Optional[str]] = {}
self._message_context_cache: Dict[str, tuple[Optional[str], List[str], List[str]]] = {}

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 make this a bounded OrderedDict LRU cache. Main deliberately caps the analogous _message_text_cache at 512 entries (e8cacb57d) because long-lived high-throughput adapters otherwise retain every unique parent message indefinitely.

@teknium1 teknium1 added 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-contained Sweeper blast radius: contained — one narrow path / opt-in / few users labels Jul 12, 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-contained Sweeper blast radius: contained — one narrow path / opt-in / few users 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 type/bug Something isn't working

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants