Skip to content

fix(feishu): keep group replies in group chats - #23729

Open
LeonSGP43 wants to merge 1 commit into
NousResearch:mainfrom
LeonSGP43:codex/23698-feishu-group-routing
Open

fix(feishu): keep group replies in group chats#23729
LeonSGP43 wants to merge 1 commit into
NousResearch:mainfrom
LeonSGP43:codex/23698-feishu-group-routing

Conversation

@LeonSGP43

Copy link
Copy Markdown
Contributor

What does this PR do?

Fixes Feishu group replies being routed through the reply API even when the inbound message was a normal group @mention rather than a Feishu topic/thread reply.

The root cause was _reply_anchor_for_event() falling back to event.message_id for ordinary Feishu group messages. That caused the adapter to call message.reply(...), which Feishu routes back to the sender DM in this scenario. This change keeps reply semantics only for true Feishu thread/topic replies and lets ordinary group responses go out through the normal group send path.

Related Issue

Fixes #23698

Type of Change

  • 🐛 Bug fix (non-breaking change that fixes an issue)
  • ✨ New feature (non-breaking change that adds functionality)
  • 🔒 Security fix
  • 📝 Documentation update
  • ✅ Tests (adding or improving test coverage)
  • ♻️ Refactor (no behavior change)
  • 🎯 New skill (bundled or hub)

Changes Made

  • Updated gateway/platforms/base.py so Feishu only returns a reply anchor for actual thread/topic replies
  • Added a regression test proving non-topic Feishu group sends use the normal group create path instead of message.reply(...)
  • Added a regression test proving ordinary Feishu group events do not request a reply anchor

How to Test

  1. Run uv run --frozen pytest -q -o addopts='' tests/gateway/test_feishu.py
  2. Confirm test_send_without_topic_metadata_creates_group_message passes
  3. Confirm test_feishu_group_events_do_not_request_reply_anchor passes

Checklist

Code

  • I've read the Contributing Guide
  • My commit messages follow Conventional Commits (fix(scope):, feat(scope):, etc.)
  • I searched for existing PRs to make sure this isn't a duplicate
  • My PR contains only changes related to this fix/feature (no unrelated commits)
  • I've run pytest tests/ -q and all tests pass
  • I've added tests for my changes (required for bug fixes, strongly encouraged for features)
  • I've tested on my platform: macOS 15

Documentation & Housekeeping

  • I've updated relevant documentation (README, docs/, docstrings) — or N/A
  • I've updated cli-config.yaml.example if I added/changed config keys — or N/A
  • I've updated CONTRIBUTING.md or AGENTS.md if I changed architecture or workflows — or N/A
  • I've considered cross-platform impact (Windows, macOS) per the compatibility guide — or N/A
  • I've updated tool descriptions/schemas if I changed tool behavior — or N/A

Screenshots / Logs

  • Targeted verification run: uv run --frozen pytest -q -o addopts='' tests/gateway/test_feishu.py
  • Focused regression subset: uv run --frozen pytest -q -o addopts='' tests/gateway/test_feishu.py -k 'topic or group_message or reply_anchor or send_without_topic_metadata'

@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 the focused Feishu routing regression fix. The production premise is still present on current main: gateway/platforms/base.py:105-107 returns event.message_id for a non-thread Feishu event, and plugins/platforms/feishu/adapter.py:4609-4621 consequently uses the reply API.

Problems

  • tests/gateway/test_feishu.py:2007 imports gateway.platforms.feishu, and line 2030 patches that old module path. Current main moved the adapter to plugins/platforms/feishu/adapter.py in 552adbe08; the added regression test needs those references migrated before it can execute.

Suggested changes

  • Update the added test's import and asyncio.to_thread patch target to plugins.platforms.feishu.adapter, following the current neighboring Feishu tests.

Automated hermes-sweeper review.

@patch.dict(os.environ, {}, clear=True)
def test_send_without_topic_metadata_creates_group_message(self):
from gateway.config import PlatformConfig
from gateway.platforms.feishu import FeishuAdapter

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.

Current main migrated the adapter to plugins.platforms.feishu.adapter in 552adbe08; this import, and the gateway.platforms.feishu.asyncio.to_thread patch at line 2030, will fail before the regression assertion runs. Please port both references to the plugin module path used by the neighboring tests.

@teknium1 teknium1 added sweeper:risk-message-delivery Sweeper risk: may drop, duplicate, misroute, or suppress messages sweeper:blast-moderate Sweeper blast radius: moderate — a subsystem or single platform labels Jul 13, 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-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.

Feishu: group message replies routed to sender's DM instead of group chat

3 participants