Skip to content

fix(gateway/feishu): bridge yaml group_rules/admins/default_group_policy to extra - #42790

Open
paid wants to merge 1 commit into
NousResearch:mainfrom
paid:paid/feishu-group-rules-bridge
Open

fix(gateway/feishu): bridge yaml group_rules/admins/default_group_policy to extra#42790
paid wants to merge 1 commit into
NousResearch:mainfrom
paid:paid/feishu-group-rules-bridge

Conversation

@paid

@paid paid commented Jun 9, 2026

Copy link
Copy Markdown

Problem

FeishuAdapter._load_settings() (gateway/platforms/feishu.py L1473-1572) reads group_rules, admins, and default_group_policy from PlatformConfig.extra, but gateway/config.py only forwards feishu.allow_bots from config.yaml — the structured fields are silently dropped.

Symptom

Users who configure per-group require_mention overrides in config.yaml (e.g. "this one group doesn't need @, all other groups still do") find the override completely ignored, even after hermes gateway restart.

Repro:

# 
feishu:
  group_rules:
    "oc_xxxxxxxxxxxxxxxxxxxxxxxxxxxx":
      require_mention: false

With FEISHU_REQUIRE_MENTION=true (the default), the per-group override is expected to make that one chat respond without @bot. Before this fix the override is dropped during yaml→adapter bridging, so the chat still requires @bot. Adapter logic itself (_require_mention_for, L4040-4044) is correct — it just never receives the rules.

Fix

Mirror the existing allow_bots bridge: explicitly forward each Feishu-specific structured key when present, so absence stays observably distinct from "empty dict". Other platforms (slack, telegram, whatsapp, signal, dingtalk, matrix) already have analogous structured-key bridges; this brings feishu in line.

# gateway/config.py, in the feishu yaml-handling block
_fs_structured_keys = ("group_rules", "admins", "default_group_policy")
if any(k in feishu_cfg for k in _fs_structured_keys):
    _, _fs_extra = _ensure_platform_extra_dict(
        platforms_data, Platform.FEISHU.value
    )

    for _k in _fs_structured_keys:
        if _k in feishu_cfg:
            _fs_extra[_k] = feishu_cfg[_k]

Tests

Two new tests in tests/gateway/test_config.py:

  • test_bridges_feishu_group_rules_from_config_yaml_to_extra — positive case covering all three structured fields (group_rules with nested require_mention + policy/allowlist, admins list, default_group_policy)
  • test_feishu_no_structured_fields_leaves_extra_clean — negative case ensuring the bridge does NOT inject empty placeholder keys when the yaml has no structured fields (presence is observably meaningful to the adapter)

Both pass; tests/gateway/test_config.py is 50/50 green after the change.

Notes

Full test suite has some pre-existing pollution-sensitive failures unrelated to this PR — they all pass when run in isolation. Verified that running test_config.py together with any of the affected files (e.g. test_telegram_model_picker.py::test_model_selected_edits_message_on_success) in isolation passes cleanly, so this PR is not the source of the pollution.

Risk

Low. The bridge only adds three keys to PlatformConfig.extra when explicitly present in config.yaml. No env-var precedence is changed. Pure additive forwarding — nothing in the existing code path is removed or reordered.

…icy to extra

FeishuAdapter._load_settings() reads group_rules/admins/default_group_policy
from PlatformConfig.extra, but gateway/config.py only forwarded allow_bots
from yaml feishu.* to extra. Per-group require_mention overrides written
under feishu.group_rules.<chat_id> in config.yaml were silently dropped.

Bridge mirrors the existing allow_bots pattern: only forward keys that are
explicitly present, so 'no group_rules' stays observably distinct from
'empty group_rules'.

Other platforms (slack, telegram, whatsapp, signal, dingtalk, matrix) have
analogous bridges for their structured fields; this brings feishu in line.

Tests: tests/gateway/test_config.py
  - test_bridges_feishu_group_rules_from_config_yaml_to_extra
  - test_feishu_no_structured_fields_leaves_extra_clean
@alt-glitch alt-glitch added type/bug Something isn't working P2 Medium — degraded but workaround exists platform/feishu Feishu / Lark adapter comp/gateway Gateway runner, session dispatch, delivery area/config Config system, migrations, profiles labels Jun 9, 2026
@liuhao1024

Copy link
Copy Markdown
Contributor

Verification comment (automated review)

Reviewed the diff — the bridge logic is correctly placed after the shared allow_bots forwarding and uses _ensure_platform_extra_dict to avoid mutating unrelated platform configs. The any(k in feishu_cfg ...) guard prevents injecting empty placeholder keys, which is important since the adapter treats key presence as meaningful.

Test coverage is thorough:

  • Structured fields (group_rules, admins, default_group_policy) correctly reach PlatformConfig.extra
  • No structured fields leaves extra clean (no empty placeholders) ✓
  • Regression test documents the original silent-drop bug ✓

No issues found. Clean, well-scoped fix.

@teknium1

Copy link
Copy Markdown
Contributor

Thanks for isolating the configuration propagation gap and adding focused regression coverage.

Problems

  • The implementation location is stale: Feishu was migrated to the bundled plugin in 560010547. Current main dispatches platform YAML hooks at gateway/config.py:1269-1304, while plugins/platforms/feishu/adapter.py:5624-5633 handles only allow_bots. The legacy core block changed by this PR no longer exists.
  • The underlying bug remains: the adapter reads group_rules, admins, and default_group_policy from extra at plugins/platforms/feishu/adapter.py:1504-1529, but the current hook returns no extras.

Suggested changes

  • Rehome the explicit-key bridge to plugins/platforms/feishu/adapter.py::_apply_yaml_config and return the selected extras; the existing dispatcher merges that return into PlatformConfig.extra at gateway/config.py:1301-1304. Port the regression tests with that implementation.

Automated hermes-sweeper review.

@teknium1 teknium1 added 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 14, 2026

@GottZ GottZ 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.

This was generated by AI during triage.

Summary

Two PRs address the same Feishu YAML propagation bug: both forward group_rules, admins, and default_group_policy into PlatformConfig.extra, but #42790 patches the removed legacy core path while #64285 implements the fix in the current plugin-owned YAML hook.

Related pull requests

  • #42790 related — (+77/-0) — superseded by #64285: The diff correctly models explicit-key forwarding and absence semantics, but it modifies the legacy gateway/config.py Feishu block that no longer exists on current main. Despite the keep_open review on #42790, the current architecture requires this logic in the bundled Feishu plugin hook, which #64285 already implements with equivalent regression coverage.
  • #64285 duplicate — (+104/-3) — merge candidate: The diff fixes the live path by returning the three structured fields from plugins/platforms/feishu/adapter.py::_apply_yaml_config, allowing the existing dispatcher to merge them into PlatformConfig.extra; its tests cover hook behavior, end-to-end adapter loading, and absent-key semantics. This addresses the ownership concern in the keep_open review on #64285.

Duplicates

#42790 and #64285 target the same underlying propagation gap and forward the same three Feishu fields; #64285 is the current-main, plugin-owned implementation and therefore supersedes #42790.

Suggested consolidation

Merge #64285 because its diff applies the fix at the current Feishu plugin ownership boundary and includes both unit and end-to-end regression coverage. Close #42790 as superseded by #64285; although its review said keep_open for salvage, the requested rehome has already been completed in #64285.

Complex graph

flowchart LR
    classDef open fill:#dbeafe,stroke:#1d4ed8,color:#1e3a8a
    classDef merged fill:#dcfce7,stroke:#15803d,color:#14532d
    classDef closed fill:#e5e7eb,stroke:#6b7280,color:#1f2937
    classDef unverified fill:#f3f4f6,stroke:#9ca3af,color:#374151
    classDef best stroke-width:3px,stroke:#b45309
    classDef target stroke-width:3px,stroke:#4338ca
    subgraph Dup42790 ["PRs duplicating each other"]
        P42790["PR #42790 (open)"]
        P64285["PR #64285 (open)"]
    end
    class P42790 open
    class P64285 open
    class P42790 target
    click P42790 "https://github.com/NousResearch/hermes-agent/pull/42790"
    click P64285 "https://github.com/NousResearch/hermes-agent/pull/64285"
Loading

Graph: solid arrow = fixes / best fix, dashed arrow = partial or unverified (see edge label); boxed group = PRs duplicating each other; amber border = best fix; indigo border = target; gray node = closed or no verify verdict yet (state tag in the node label).

Cross-PR triage: Reviewed 2 pull requests and 0 issues in this complex. Each diff was read against this issue; Assessment working set: 11 kB of PR diffs, 5 kB of issue/PR text, 3 kB of discussion (5 comments), 1 verify verdict. verdicts reflect diff content, not PR titles. Part of an automated triage batch.

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 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-compatibility Sweeper risk: may break existing users, config, migrations, defaults, or upgrades type/bug Something isn't working

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants