Skip to content

fix(feishu): keep topic replies in active threads - #18385

Open
315zuicai wants to merge 5 commits into
NousResearch:mainfrom
315zuicai:fix/feishu-topic-follow-replies
Open

fix(feishu): keep topic replies in active threads#18385
315zuicai wants to merge 5 commits into
NousResearch:mainfrom
315zuicai:fix/feishu-topic-follow-replies

Conversation

@315zuicai

@315zuicai 315zuicai commented May 1, 2026

Copy link
Copy Markdown

Summary

  • keep Feishu/Lark topic replies in the active topic by carrying the inbound om_... message id as reply_to_message_id
  • use Feishu reply API metadata fallback so progress/status/final sends can reply with reply_in_thread=True instead of only carrying omt_... topic id
  • add opt-in Feishu topic follow mode (feishu.thread_follow_enabled, feishu.thread_follow_ttl_seconds) so a mentioned topic can continue without repeated @mentions until TTL expiry
  • refresh active topic follow windows on accepted unmentioned human messages, making the TTL an inactivity timeout instead of a fixed window from the initial @mention
  • route post-stream MEDIA:<path> image/file delivery through the same Feishu topic metadata helper so native attachments also reply inside the originating topic

Test Plan

  • python -m py_compile gateway/platforms/feishu.py tests/gateway/test_feishu_bot_admission.py
  • ./venv/bin/python -m py_compile gateway/run.py gateway/platforms/feishu.py gateway/config.py tests/gateway/test_feishu_bot_admission.py
  • ./venv/bin/python -m pytest tests/gateway/test_feishu.py tests/gateway/test_feishu_bot_admission.py tests/gateway/test_run_progress_topics.py -q
  • /root/.hermes/scripts/hermes-feishu-topic-patch-verify.sh

Result: 292 passed, 8 dependency deprecation warnings.

Carry the inbound Feishu message id through topic metadata so replies use the reply API with reply_in_thread, and add an opt-in thread-follow window that lets a mentioned topic continue without repeated @mentions.
@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 May 1, 2026
@alt-glitch

Copy link
Copy Markdown
Collaborator

Related to #17877 and #18121 which address the same Feishu topic thread reply issue. This PR adds thread_follow mode on top. Maintainers should decide which approach to merge — #18121 claims to be comprehensive (A+B+C+D layers).

@alt-glitch

Copy link
Copy Markdown
Collaborator

Related to #17877 and #18121

- Avoid refreshing topic-follow TTL from unmentioned follow-up messages
- Carry Feishu reply_to_message_id through /background result sends
- Prune expired topic-follow entries and cover edge cases with tests
@315zuicai

Copy link
Copy Markdown
Author

Follow-up review + hardening update for the Feishu topic-thread fix.

What changed after the review:

  • Hardened thread-follow TTL behavior: unmentioned follow-up messages inside an active Feishu topic are admitted within the original TTL, but they do not refresh/extend the TTL. Only an explicit new mention re-activates or extends the follow window.
  • Preserved the real inbound Feishu message id (om_...) as reply_to_message_id for topic replies. Feishu topic sends now use the message reply API with reply_in_thread=True, rather than relying on the topic id (omt_...) alone.
  • Propagated reply_to_message_id into /background task metadata so completion/error/media messages stay in the original Feishu topic.
  • Added pruning for expired thread-follow entries and switched the follow window to monotonic time.
  • Added/strengthened regression coverage for topic follow expiry/non-refresh and background topic replies.

Verification performed locally:

  • py_compile for gateway/run.py, gateway/platforms/feishu.py, and gateway/config.py.
  • Focused Feishu/topic pytest verification via ~/.hermes/scripts/hermes-feishu-topic-patch-verify.sh:
    • 290 passed, 8 warnings in 20.62s
  • Static scan of added diff lines for common secret/injection/eval/pickle/SQL-formatting patterns: no findings.
  • Independent review pass was performed; the main actionable finding was the TTL non-refresh boundary, now covered by tests.

Known unrelated local suite note:

  • A broader tests/gateway run exposed pre-existing/unrelated failures/timeouts in approval and Teams tests (test_blocking_approval_approve_once, test_blocking_approval_deny, test_send_typing). The focused Feishu/topic verification above passes.

Deployment safety note:

  • For local deployments where this PR is not yet merged, I also prepared a local patch-stack + verification wrapper so upgrades fail loudly if this behavior would be lost. That wrapper is intentionally outside this PR, but the source-level regression tests in this PR cover the important behavior.

@315zuicai

Copy link
Copy Markdown
Author

Follow-up verification after review

I did a post-PR verification pass and pushed one hardening commit (a697aa0c9) to the PR branch.

What changed in the follow-up commit

  • Carries Feishu reply_to_message_id through additional topic reply paths, including progress/status/model-picker/background-task flows.
  • Makes the adapter fall back to metadata.reply_to_message_id when reply_to is not explicitly passed, so Feishu topic replies use the real om_... message id instead of only the omt_... topic id.
  • Adds opt-in thread-follow admission tests covering TTL, expiry, non-refresh on unmentioned messages, pruning, and group-policy boundaries.
  • Adds regression tests for topic progress/background replies carrying both thread_id and reply_to_message_id.

Verification

  • Static added-line scan: no hardcoded secrets, shell injection patterns, eval/exec, pickle, or simple SQL formatting patterns found.
  • venv/bin/python -m py_compile gateway/config.py gateway/platforms/feishu.py gateway/run.py
  • venv/bin/python -m pytest -q -o 'addopts=' tests/gateway/test_feishu.py tests/gateway/test_feishu_bot_admission.py tests/gateway/test_run_progress_topics.py ✅ — 290 passed, 2 warnings.
  • Independent reviewer pass: no blocking correctness/security issues found.

Non-blocking follow-ups noted

  • Some long-lived watcher/update/restart notification paths may still only have thread_id after process boundaries because the original Feishu message id is not persisted there.
  • _active_thread_follows is opportunistically pruned; a future LRU/max-size cap could further bound memory in very large workspaces.

@SnowWarri0r

Copy link
Copy Markdown

Reproduces in another shared-gateway deployment — exact symptom: bot @-mentioned inside a topic group reply ends up in a brand-new sub-thread instead of staying in the active topic. The dual fix here (carry om_... id forward + opt-in thread_follow TTL) maps cleanly to the two pain points: stale topic on first reply, and forced re-@ on every subsequent turn. Would help to see this land.

@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 detailed Feishu threading work. Current main already covers the core reply-in-topic behavior through 441ef75d1 and the plugin adapter's metadata reply path (plugins/platforms/feishu/adapter.py:4609-4621), but the proposed opt-in follow window remains distinct.

Problems

  • gateway/platforms/feishu.py:3804 keys follow state only from message.thread_id. Admission runs before inbound normalization (plugins/platforms/feishu/adapter.py:2536), while the normalized topic ID explicitly falls back to message.root_id (plugins/platforms/feishu/adapter.py:3252). Root-ID-only topic events therefore cannot activate or consume the follow TTL.
  • The PR modifies the former adapter path. Feishu now owns YAML handling through plugins/platforms/feishu/adapter.py:5618-5654; the current patch cannot be cleanly applied to main.
  • gateway/platforms/feishu.py:1528 and :1532 add new non-secret HERMES_* configuration fallbacks, contrary to the config.yaml-only policy in AGENTS.md:102-106.

Suggested changes

  • Use thread_id or root_id for the admission-time follow key and add an end-to-end adapter test for a root-ID-only topic event.
  • Salvage the remaining follow-window feature into the Feishu plugin/config hook, using config values rather than new environment variables.

This is an automated hermes-sweeper review.


def _thread_follow_key(self, message: Any) -> Optional[tuple[str, str]]:
chat_id = str(getattr(message, "chat_id", "") or "").strip()
thread_id = str(getattr(message, "thread_id", "") or "").strip()

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.

Topic events can expose root_id without thread_id. _admit runs before _process_inbound_message, where current main normalizes the topic as thread_id or root_id; this leaves the follow key empty and prevents the TTL from activating. Use the same root fallback here and cover the full inbound path.

@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-moderate Sweeper blast radius: moderate — a subsystem or single platform labels Jul 12, 2026
@alt-glitch alt-glitch added needs-decision Awaiting maintainer decision before any implementation comp/tools Tool registry, model_tools, toolsets tool/terminal Terminal execution and process management sweeper:risk-session-state Sweeper risk: may lose/corrupt/mis-associate session or context state labels Jul 18, 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 comp/tools Tool registry, model_tools, toolsets needs-decision Awaiting maintainer decision before any implementation 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 tool/terminal Terminal execution and process management type/bug Something isn't working

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants