Skip to content

feat(feishu): complete multiplex_profiles support — multi-bot group discussion - #69904

Open
zmlgit wants to merge 5 commits into
NousResearch:mainfrom
zmlgit:feat/feishu-multiplex-multi-bot-discussion
Open

feat(feishu): complete multiplex_profiles support — multi-bot group discussion#69904
zmlgit wants to merge 5 commits into
NousResearch:mainfrom
zmlgit:feat/feishu-multiplex-multi-bot-discussion

Conversation

@zmlgit

@zmlgit zmlgit commented Jul 23, 2026

Copy link
Copy Markdown

Summary

Enables distinct Feishu bot identities (separate apps, avatars, names) to coexist as a discussion panel inside one multiplexing gateway — each bot answering user questions in shared group chats from its own identity. profile_routes cannot express this (it routes messages from one bot to different personas; users see a single bot talking to itself, defeating the "panel of experts" UX). One-process-per-profile works but loses the in-process coordination needed for orchestrated multi-bot turn-taking (architect → coder → reviewer sequencing) — and multiplies operational cost N-fold.

multiplex_profiles + per-profile feishu credentials is the natural fit. Upstream code already implies support:

# gateway/config.py
PORT_BINDING_CONDITIONAL_MODES = {"feishu": "webhook"}

This rule says "feishu doesn't bind a port in WS mode, so it's safe for secondary profiles under multiplexing". But the implementation doesn't follow through — feishu adapters silently fail to start in secondary profiles. This PR completes the implementation the conditional-mode rule already promises.

Use case

A team wants multiple specialized bots in one Feishu group chat — e.g. an architect bot, a coder bot, and a reviewer bot — each visible to users as a distinct entity (own avatar, name, app identity). When a user asks a question, the relevant bot(s) answer from their own identity. This is the "panel of experts" UX common in team collaboration tools.

This is fundamentally different from "one bot, multiple personas" (which profile_routes already handles): users must see different bot identities, not one bot role-playing.

Why this PR (and not Mode 1 / profile_routes)

Approach Multi-bot identity In-process coordination Ops cost
profile_routes ❌ one bot, N personas n/a low
one-process-per-profile (Mode 1) ❌ (need IPC) N× systemctl/memory/logs
multiplex_profiles + this PR ✅ (_profile_adapters[name][platform]) low

For "panel of experts in one group chat", only multiplex_profiles satisfies both axes.

What's in this PR (3 commits + tests)

  1. feat(feishu): seed adapter extra from YAML config for multi-profile support

    • _apply_yaml_config translates the per-profile feishu: YAML block into the FEISHU_* env-vars / extra dict the plugin loader and FeishuAdapterSettings consume.
    • Without this, a secondary profile's feishu block is silently ignored: the loader still requires FEISHU_APP_ID env var (set globally only on the default profile), so the plugin never registers and 0 feishu adapters start in secondary profiles.
  2. fix(feishu): restore _ThreadLocalLoopProxy for multi-profile WS isolation

    • Replaces lark_oapi.ws.Client's module-global loop with a thread-local proxy so each profile's worker thread sees its own loop.
  3. fix(feishu): patch SDK client methods to use per-instance hermes_loop

    • Patches the SDK client method dispatch to reach for self._hermes_loop (per-adapter) instead of the module global. Together with (2), this is what stops the second profile's WS connect from tripping RuntimeError: Task ... got Future ... attached to a different loop.
  4. test(feishu): regression guards (tests/gateway/test_feishu_multiplex_support.py)

    • 5 tests covering the YAML-config plumbing (no env leak of app_secret, empty-block fallback, all 6 credential keys flow into extra) and the loop-isolation mechanism (_ThreadLocalLoopProxy class exists + _hermes_loop references in adapter source).

Verification evidence (production-equivalent)

I tested the three commits together on a fork of the user's production setup (9 feishu apps, multiplex_profiles=true, real per-profile YAML config). Compared pure origin/main against this PR:

Build Result
origin/main + production config Gateway running with 1 platform(s)0 feishu adapters started, silent
+ this PR Gateway running with 11 platform(s)8/9 feishu profiles connected cleanly, 5 min stable, 0 errors

Representative log diff:

# BEFORE (origin/main only):
2026-07-23 09:57:02,893 INFO gateway.run: Gateway running with 1 platform(s)
2026-07-23 09:57:02,896 INFO gateway.run: Channel directory built: 0 target(s)
[no feishu attempt at all — silent skip]

# AFTER (+ this PR):
2026-07-23 11:35:42,680 INFO gateway.run: ✓ feishu connected (profile: ceo)
2026-07-23 11:35:46,664 INFO gateway.run: ✓ feishu connected (profile: coder)
2026-07-23 11:35:49,843 INFO gateway.run: ✓ feishu connected (profile: dba)
2026-07-23 11:35:52,811 INFO gateway.run: ✓ feishu connected (profile: designer)
2026-07-23 11:35:56,304 INFO gateway.run: ✓ feishu connected (profile: pm)
2026-07-23 11:36:00,904 INFO gateway.run: ✓ feishu connected (profile: reviewer)
2026-07-23 11:36:05,849 INFO gateway.run: ✓ feishu connected (profile: shipper)
2026-07-23 11:36:09,403 INFO gateway.run: ✓ feishu connected (profile: tester)
2026-07-23 11:36:09,406 INFO gateway.run: Gateway running with 11 platform(s)
[0 ERROR / 0 RuntimeError / 0 UnscopedSecretError during 5-minute stability window]

The intermediate state (only commit 1 applied, without 2+3) is the most informative: feishu starts per-profile but the second profile's WS connect immediately fails with the exact Task attached to a different loop error commit 2+3 fix.

Intentionally excluded from this PR

To keep the PR scope tight and review-friendly:

  • WS IP failover for msg-frontier DNS black-holes — was part of the original internal branch, has a BaseEventContext.create_connection() got multiple values for argument 'host' conflict on current main. Will file as a follow-up PR after fixing the host-arg collision.
  • Multiplex secret-scope fallback for UnscopedSecretError — the 5-minute verification window did not surface this error path once the WS loop isolation landed. Will revisit if a longer-running repro shows it firing.

Backward compatibility

  • Default-profile/single-profile behavior is byte-identical when multiplex_profiles is off (the PORT_BINDING_CONDITIONAL_MODES rule already permits feishu in secondary profiles only when multiplexing is active).
  • The new _apply_yaml_config returns None for an empty feishu block, so single-profile configs that don't set a feishu block see no behavior change.
  • The _ThreadLocalLoopProxy and _hermes_loop patches are no-ops in single-profile mode (only one WS client, no contention).

Related

张满良 added 4 commits July 23, 2026 13:37
…upport

Allow Feishu adapter to read its per-instance config (app_id, app_secret,
domain, connection_mode, encrypt_key, verification_token, allow_bots)
from the YAML "extra" block instead of requiring process-wide env vars.
Enables running multiple Feishu profiles side-by-side in a multiplex
gateway (each profile gets its own app credentials).

Changes:
- gateway/run.py: drop feishu from _PORT_BINDING_PLATFORM_VALUES so a
  secondary profile binding doesn't trip the hard-error port-collision
  gate (multi-profile feishu runs N adapters in one process, no port).
- plugins/platforms/feishu/adapter.py:
  * _load_settings: read allow_bots from extra (with env fallback) so
    each profile can have its own bot-admission policy.
  * _apply_yaml_config: seed and return an extra dict carrying the
    YAML-resolved feishu keys, so PlatformConfig.extra flows through
    to adapter instantiation. Still seeds FEISHU_ALLOW_BOTS env for
    the gateway auth bypass path (unchanged).
- tests/gateway/test_config.py: 2 new tests — feishu YAML enables the
  platform + extra group_rules propagate.
- tests/gateway/test_feishu_bot_admission.py: flip the
  "ignores extra" test to "prefers extra" (matches new behavior).
- tests/gateway/test_multiplex_adapter_registry.py: new test asserting
  a secondary feishu profile binds without raising.
- package-lock.json: stale dep-tree cleanup.
…tion

Without the thread-local loop proxy, each feishu adapter overwrites the
Lark SDK's module-global event loop, causing 'Future attached to a
different loop' crashes when multiplexing multiple profile adapters.
Restored from 58ca5f7.
… instead of module global

The lark_oapi SDK's ws/client.py references a module-level loop global for
loop.create_task(...) calls inside _connect and _receive_message_loop.
In multiplex mode several adapter threads start near-simultaneously and each
overwrites that global, so a later reconnect on an earlier client schedules
coroutines on the wrong loop ('Task got Future attached to a different loop').

Previously _install_thread_local_ws_loop_proxy() made loop a thread-local, but
the SDK still used the module global. This patch monkey-patches the three SDK
methods that use the global (start, _connect, _receive_message_loop) to reach
for self._hermes_loop instead.
Three categories of regression risk for the multi-profile feishu feature,
each with a focused test that fails loudly when its mechanism breaks:

FeishuMultiplexYamlConfigTest (3 tests):
  - _apply_yaml_config seeds app creds into config.extra (the per-profile
    YAML→adapter plumbing that lets a secondary profile declare its own
    feishu app without polluting process-wide env).
  - _apply_yaml_config does NOT leak app_secret into os.environ (under a
    multiplexer that would expose it to every other profile's turn).
  - empty feishu block returns None so legacy single-profile behavior is
    byte-identical.

FeishuMultiplexLoopIsolationTest (2 tests):
  - _ThreadLocalLoopProxy class exists at module scope (replaces the SDK's
    module-global loop so each worker thread sees its own).
  - FeishuAdapter source references self._hermes_loop (verifies the SDK
    client method patches reach for the per-instance loop slot rather than
    the module global). Without either half, production hits:
    RuntimeError: Task ... got Future ... attached to a different loop
@zmlgit
zmlgit requested a review from a team July 23, 2026 05:45
@alt-glitch alt-glitch added type/feature New feature or request P3 Low — cosmetic, nice to have comp/plugins Plugin system and bundled plugins platform/feishu Feishu / Lark adapter area/config Config system, migrations, profiles area/profiles Multi-profile isolation, HERMES_HOME scoping sweeper:risk-message-delivery Sweeper risk: may drop, duplicate, misroute, or suppress messages labels Jul 23, 2026
… WS death

The _ThreadLocalLoopProxy monkey-patch replaces the SDK's receive loop but
inherited the SDK's 'raise e when _auto_reconnect=False' branch. When WS
closed normally (ConnectionClosedOK, code 1000), the task re-raised into
the asyncio void ('Task exception was never retrieved') and never
reconnected. Gateway kept reporting 'feishu connected' from startup while
the actual WS had been dead for hours — users saw 0 inbound message
delivery until manual gateway restart.

Fix: always call self._reconnect() in the patched loop, ignoring
_auto_reconnect (the adapter shutdown path uses _auto_reconnect=False but
mid-flight closes shouldn't honor that). Reconnect failures are logged
loudly instead of raised, so the task exits cleanly and a watchdog can
detect '0 inbound feishu messages' as the failure signal.

@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 narrowing the Feishu multiplex work and including config coverage. The YAML-to-PlatformConfig.extra gap is still present on current main: plugins/platforms/feishu/adapter.py:5799-5808 returns no seeded credentials, while gateway/config.py:1668-1678 would merge such a return into extra.

Problems

  • plugins/platforms/feishu/adapter.py:1460-1462 always reconnects after a receive-loop exit. Current shutdown deliberately disables SDK reconnect before calling _disconnect (plugins/platforms/feishu/adapter.py:1792-1807), so this can reconnect during teardown.
  • tests/gateway/test_feishu_multiplex_support.py:143-155 reads source and checks for _hermes_loop; it does not validate two isolated live loop/client paths or shutdown behavior.
  • The new per-profile Feishu configuration is undocumented. Existing Feishu docs describe env setup and platforms.feishu.extra, not this multiplex topology.

Suggested changes

  • Honor _auto_reconnect in the patched receive loop and test that shutdown cannot reconnect.
  • Replace structural source checks with concurrent fake-SDK behavioral coverage for isolated loops and reconnect/shutdown.
  • Add user-facing multiplex Feishu configuration documentation.

Automated hermes-sweeper review.

ws_client_module.logger.warning(
self._fmt_log("disconnect after receive loop exit failed: {}", disconnect_err),
)
try:

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 unconditionally reconnects even after FeishuAdapter.disconnect() has deliberately set _auto_reconnect=False before calling the SDK _disconnect (current main: plugins/platforms/feishu/adapter.py:1792-1807). Please retain the shutdown gate and add a regression test that a clean adapter shutdown cannot initiate a new connection.

# Verify by reading the adapter source file directly —
# inspect.getsource(FeishuAdapter) truncates on classes this large.
import plugins.platforms.feishu.adapter as _mod
with open(_mod.__file__, encoding="utf-8") as fh:

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 is a source-shape assertion, not a loop-isolation test. Exercise two fake SDK clients concurrently and assert their scheduled tasks stay on their own loops; also cover the shutdown/reconnect boundary. That will catch wiring regressions without depending on a private symbol or source text.

@teknium1 teknium1 added sweeper:risk-session-state Sweeper risk: may lose/corrupt/mis-associate session or context state 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 30, 2026
@zmlgit

zmlgit commented Jul 31, 2026

Copy link
Copy Markdown
Author

Update — shutdown reconnect race fixed + behavioral tests + docs

Committed b05b00a03 on pr-69904.

Fix

The patched receive loop's exception handler now checks _auto_reconnect before calling _reconnect. During shutdown, _disable_websocket_auto_reconnect sets _auto_reconnect = False before closing the connection, so the receive loop's exception handler exits cleanly instead of racing to resuscitate a dead connection.

Tests

Replaced the source-text _hermes_loop check (banned by AGENTS.md: "Never read source code in tests") with two behavioral fake-SDK tests:

  1. test_run_official_feishu_ws_client_isolates_hermes_loop_per_instance — runs the real _run_official_feishu_ws_client against a fake SDK for two clients, asserts each gets its own _hermes_loop + bound patched methods.

  2. test_patched_receive_loop_skips_reconnect_when_auto_reconnect_disabled — pre-flips _auto_reconnect = False, runs the real patched loop with a fake conn whose recv() raises, asserts _reconnect is NOT called (0 calls) and _disconnect IS called (cleanup runs).

All 230 tests across test_feishu.py, test_feishu_multiplex_support.py, test_compress_command.py pass.

Docs

Added a "Multiple Feishu Apps in One Gateway (Multiplex)" section to website/docs/user-guide/messaging/feishu.md documenting per-profile YAML configuration, loop isolation, and shutdown behavior.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

area/config Config system, migrations, profiles area/profiles Multi-profile isolation, HERMES_HOME scoping comp/plugins Plugin system and bundled plugins P3 Low — cosmetic, nice to have 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/feature New feature or request

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants