fix(feishu): honor platform extra reply_in_thread and forbid DM thread mode - #60916
fix(feishu): honor platform extra reply_in_thread and forbid DM thread mode#60916Suvern wants to merge 2 commits into
Conversation
8305a98 to
da5fe86
Compare
tonydwb
left a comment
There was a problem hiding this comment.
Code Review Summary
Verdict: Comment (high surface area — 297 additions, 24 files, Feishu reply threading)
Scope
Honors platform extra reply_in_thread for Feishu and forbids DM thread mode. 297 additions across many files.
Observations
- Platform-specific threading logic for Feishu.
- Cross-platform compatibility fix — thread mode handling differs for DM vs group.
- No obvious security issues.
Recommendation
297 additions warrants human reviewer sign-off. The thread mode rules should be verified for correctness across different Feishu chat types.
Reviewed by Hermes Agent
teknium1
left a comment
There was a problem hiding this comment.
Thanks for addressing a real Feishu configuration gap. Current main still derives reply_in_thread only from metadata at plugins/platforms/feishu/adapter.py:4612.
Problems
gateway/run.py:14143addschat_typeonly to the runner helper. Normal final replies usegateway/platforms/base.py:55-77to build metadata and send it atgateway/platforms/base.py:5005-5010; that helper emits onlythread_idfor Feishu. Consequently the new DM guard does not receive a chat class on the primary response path.- The equality detector added near
plugins/platforms/feishu/adapter.py:4638is not Feishu-safe as written: inbound processing mapsroot_idinto boththread_idandreply_to_message_idatplugins/platforms/feishu/adapter.py:3252-3257. A genuine root-topic reply can satisfy the proposed synthetic condition.
Suggested changes
- Propagate chat type through the shared metadata path and test an actual p2p final reply end to end.
- Narrow or remove the synthetic-thread heuristic, with coverage for a genuine root-id topic event.
Automated hermes-sweeper review.
| # decisions (e.g. Feishu must not reply-in-thread on DMs even when | ||
| # the gateway stamped a routing thread_id). Each adapter falls back | ||
| # to its own resolution when the key is absent. | ||
| if chat_type and "chat_type" not in metadata: |
There was a problem hiding this comment.
This does not cover normal final replies: BasePlatformAdapter builds their metadata via gateway/platforms/base.py:_thread_metadata_for_source, which currently emits only thread_id, then sends it at base.py:5005-5010. Please propagate the source chat type through that shared path (and test a p2p final response), otherwise the DM guard never sees chat_type for the primary delivery path.
| reply_in_thread = bool((metadata or {}).get("thread_id")) | ||
| if effective_reply_to: | ||
| anchored_thread_id = md.get("reply_to_message_id") | ||
| synthetic_thread_only = bool(thread_id) and bool(anchored_thread_id) and str( |
There was a problem hiding this comment.
Equality is not sufficient to identify a synthetic Feishu thread: current inbound handling assigns root_id to both thread_id and reply_to_message_id (adapter.py:3252-3257). This can classify a genuine root-topic reply as synthetic and route it to chat_id; please narrow the predicate or add a Feishu-specific discriminator.
Suvern
left a comment
There was a problem hiding this comment.
Thanks for catching both — you were right on both counts. Pushed a follow-up commit that addresses each problem.
1. chat_type didn't reach the primary final-reply path.
You called it: I only added chat_type to gateway/run.py::_thread_metadata_for_target, but the primary path routes through gateway/platforms/base.py::_thread_metadata_for_source (the metadata builder the final reply actually consumes at base.py:5005-5010). I added the same source.chat_type forward in the base helper so the DM guard now fires on normal sends, not just the runner path.
2. Synthetic-thread detector suppressed real root-topic replies.
I had the reply_to_message_id == thread_id heuristic modeled on the Slack adapter, but you spotted the asymmetry: Feishu inbound maps root_id into BOTH fields (adapter.py ~3252), so a genuine root-topic reply satisfies that condition. The heuristic would have silently downgraded a real, working feature. I dropped the detector entirely. The two sufficient signals are chat_type (DM hard rule) and extra.reply_in_thread (operator opt-out) — nothing else.
Test changes:
- Replaced
test_synthetic_thread_does_not_open_reply_chainwithtest_root_topic_reply_with_equality_still_uses_reply_apito pin the new (correct) behavior. - Added
test_dm_metadata_via_base_helper_keeps_p2p_final_reply_flatthat drives_thread_metadata_for_sourcedirectly — that's the end-to-end p2p final-reply path you asked for.
pytest tests/gateway/test_feishu.py tests/gateway/test_feishu_reply_in_thread.py → 213/213 (up from 209; +5 cases, all passing, no regressions in the existing suite).
…d mode
The Feishu gateway adapter read ``reply_in_thread = bool(metadata.get("thread_id"))``,
ignoring the operator's ``platform_cfg.extra.reply_in_thread`` knob that the
Slack adapter already respects (slack/adapter.py:1454). The legacy shape made
it impossible to disable reply-in-thread globally from config, and it also
forced reply-in-thread=true on Feishu p2p chats — the reply API renders
reply_in_thread=true as a fresh discussion surface in DMs, which the client
then displays as "the bot started a thread" UX.
Three changes:
* ``plugins/platforms/feishu/adapter.py::_send_raw_message`` now resolves
``reply_in_thread`` from ``self.config.extra.get("reply_in_thread", True)``,
mirroring the Slack adapter. Two chat-class guards are layered on top:
``chat_type in {"p2p","dm",...}`` from forwarded metadata forces
``reply_in_thread=False`` regardless of config (the Feishu reply API has no
way to opt out at the chat class level), and a Slack-style synthetic-thread
detector (``reply_to_message_id == thread_id``) prevents the adapter from
opening a fresh reply chain when the gateway leaked a routing-only topic
stamp. The DM guard extends to the create fallback so a leaked thread_id
doesn't end up as the create API's receive_id either.
* ``gateway/run.py::_thread_metadata_for_target`` now forwards
``chat_type`` into the metadata dict so adapters can apply per-chat-class
routing rules without an extra round-trip.
* New ``tests/gateway/test_feishu_reply_in_thread.py`` pins the four
decisions (default thread, extra-off, DM hard rule, synthetic thread).
Verified: 209 of 209 tests pass across test_feishu.py + the new file. The
3 pre-existing failures in test_feishu_approval_buttons.py are not related
(verified by stashing this branch and re-running the same tests against
origin/main HEAD).
…reply path and drop unsafe synthetic-thread detector - gateway/platforms/base.py::_thread_metadata_for_source: forward source.chat_type into the metadata dict. This is the metadata builder the primary final-reply path actually uses (base.py ~5005), so without this change the DM guard in the adapter never fires for normal sends — only the runner helper was carrying chat_type. - plugins/platforms/feishu/adapter.py::_send_raw_message: remove the reply_to_message_id == thread_id synthetic-thread detector. Inbound processing maps root_id into BOTH fields for genuine root-topic replies (~adapter.py:3252), so the equality check would mistakenly suppress real topic replies. Leave the chat_type DM rule and the extra.reply_in_thread knob as the two sufficient signals. - tests/gateway/test_feishu_reply_in_thread.py: replace the synthetic case with a regression that pins the root-topic equality behavior (must still use the reply API) and an E2E test that drives the base helper directly to confirm chat_type reaches the adapter on the primary final-reply path.
d8f1b76 to
52b1278
Compare
|
Rebased onto
|
|
Friendly bump 👋 — this has been rebased onto current Happy to adjust anything further, or rebase again if main has moved. |
|
Friendly bump 👋 — rebased onto current
Happy to iterate further or rebase again if |
Summary
Fix the Feishu gateway adapter so it honors the operator's
platform_cfg.extra.reply_in_threadconfig knob (the way the Slack adapteralready does at
plugins/platforms/slack/adapter.py:1454) and stop it fromimplicitly opening a fresh reply thread on p2p / DM chats — where the Feishu
reply API renders
reply_in_thread=trueas a new discussion surface that theclient displays as "the bot started a thread".
The current code resolves
reply_in_threadpurely frombool(metadata.get("thread_id")), which:extra.reply_in_threadknob silently inert for Feishuoperators, and
reply_in_thread=trueon every DM where the gateway has stamped arouting-only
thread_id, producing the unwanted UX.What changed
plugins/platforms/feishu/adapter.py—_send_raw_messageResolves
reply_in_threadfromself.config.extra.get("reply_in_thread", True), with two layered guards and asynthetic-thread detector aligned with the Slack adapter's model:
chat_type in {"p2p", "dm", "direct_message", ...}is in forwarded metadata,
reply_in_threadis forced toFalseregardless of the config value. The Feishu reply API has no way to opt out
per chat class, so the adapter has to enforce this itself.
reply_to_message_id == thread_id(the gateway leaked a routing-only topic stamp that reuses the reply
target's id), the adapter skips the reply API and falls through to
chat.send, the same way Slack does for the same situation. Thisprevents the adapter from opening a fresh reply chain on a synthetic stamp.
fallback so a leaked
thread_iddoesn't end up as the create API'sreceive_ideither.gateway/run.py—_thread_metadata_for_targetForwards
chat_typeinto the metadata dict so adapters can apply perchat-class routing rules without an extra round-trip. This is the minimum
surface change needed to give the adapter the signal it now needs.
tests/gateway/test_feishu_reply_in_thread.py(new)Pins the four decisions the adapter now has to make, with real imports
against a temp
HERMES_HOME(no mocks of the resolver logic):test_default_reply_in_thread_true_keeps_threaded_topic_behaviour— noextraconfig, realthread_idin metadata → reply API used.test_extra_reply_in_thread_false_disables_reply_chain— operator setsextra.reply_in_thread=false→ reply API skipped even with a realthread_id.test_dm_chat_class_hard_forces_reply_in_thread_false—chat_type=p2pplus a leaked
thread_id→ both the reply path and the create fallbackuse
chat_idasreceive_id, neverthread_id.test_synthetic_thread_does_not_open_reply_chain—reply_to_message_id == thread_id→ reply API skipped, falls through tochat.send.How to test
The three pre-existing failures in
tests/gateway/test_feishu_approval_buttons.pyare reproducible on
origin/mainHEAD without this branch checked out(verified via
git stash+ re-run), so they are unrelated baseline breakageand not caused by this PR.
For the manual reproduction path the original
reply_in_thread = bool(metadata.get("thread_id"))shape was triggering:
~/.hermes/config.yamlwithchannels.feishu.extra.reply_in_thread: false.thread_id.thread" UX), and
extra.reply_in_thread=falsehas no effect.and the
extraknob is honored on group chats as well.What platforms you tested on
pytestruns cleanly.reply_in_thread=truerendering claim iscross-referenced against the existing Slack adapter's analogous guard at
plugins/platforms/slack/adapter.py:1454to keep both adapters consistent.agnostic Python.
Related
plugins/platforms/feishu/adapter.py::_send_raw_messagenever read
self.config.extra, which made the Feishu platform the onlyone in
plugins/platforms/*whosereply_in_threadknob was inert.patrick-fu:Add configurable Feishu thread replies(only flipsthe config read; does not cover DM / synthetic-thread.)
T0UGH:[codex] Add Feishu thread reply toggle(touches an oldergateway/platforms/feishu.pylayout.)This PR is a strict superset — both behaviours, plus the DM guard and the
synthetic-thread detector — but the underlying mechanism
(
extra.reply_in_thread) is the same, so review conflict should beresolvable by merging the touched lines if/when this lands first.
Checklist
(
fix(feishu): honor platform extra reply_in_thread and forbid DM thread mode)fix/<description>pytestgreen on touched files (rebase onto currentorigin/main,full
tests/gateway/test_feishu.py+ new file yield 212 / 212passed, 0 regressions on this branch); pre-existing failures in
test_feishu_approval_buttons.pyare baseline and unrelatedHERMES_*env vars