Skip to content

fix(feishu): respect explicit enabled: false when env vars are present (#47804) - #47807

Open
liuhao1024 wants to merge 2 commits into
NousResearch:mainfrom
liuhao1024:fix/47804-feishu-env-override-respects-enabled
Open

fix(feishu): respect explicit enabled: false when env vars are present (#47804)#47807
liuhao1024 wants to merge 2 commits into
NousResearch:mainfrom
liuhao1024:fix/47804-feishu-env-override-respects-enabled

Conversation

@liuhao1024

Copy link
Copy Markdown
Contributor

What does this PR do?

Fixes Feishu platform env var override ignoring explicit enabled: false in config.yaml. When FEISHU_APP_ID and FEISHU_APP_SECRET env vars are present, _apply_env_overrides() unconditionally forced enabled = True, overriding the user's explicit config. This caused sub-profiles to attempt Feishu connections and trigger app_id conflicts in multi-profile deployments.

Related Issue

Fixes #47804

Type of Change

  • 🐛 Bug fix (non-breaking change that fixes an issue)

Changes Made

  • gateway/config.py: Replace manual PlatformConfig() + enabled = True with _enable_from_env(Platform.FEISHU) which checks _enabled_explicit flag before enabling (same pattern used by Telegram)
  • tests/gateway/test_config.py: Add TestFeishuEnvOverrideRespectsExplicitEnabled with 4 regression tests covering: explicit disabled stays disabled, no explicit enables, env var population, and existing config without enabled key

How to Test

  1. Set FEISHU_APP_ID and FEISHU_APP_SECRET env vars
  2. Create a config.yaml with platforms.feishu.enabled: false
  3. Start the gateway — Feishu should NOT be enabled
  4. Remove enabled: false from config — Feishu should be enabled via env vars
  5. Run pytest tests/gateway/test_config.py::TestFeishuEnvOverrideRespectsExplicitEnabled -v

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

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

Code Intelligence

  • Analyzed: gateway/config.py::_apply_env_overrides() (Feishu section, lines 1761-1785)
  • Blast radius: LOW — single platform's env override path, no control flow changes
  • Related patterns: _enable_from_env() helper already used by Telegram (line 1365); Slack uses equivalent enabled_was_explicit check (line 1500)

When a user's config.yaml explicitly sets feishu.enabled: false,
the _apply_env_overrides() function was unconditionally forcing
enabled=True when FEISHU_APP_ID/FEISHU_APP_SECRET env vars existed.
This ignored the user's explicit config, causing sub-profiles to
attempt Feishu connections and trigger app_id conflicts.

Use the existing _enable_from_env() helper (already used by Telegram)
which checks the _enabled_explicit flag before enabling. Slack uses
the same pattern via its own enabled_was_explicit check.

4 regression tests added.
@alt-glitch alt-glitch added type/bug Something isn't working comp/gateway Gateway runner, session dispatch, delivery platform/feishu Feishu / Lark adapter area/config Config system, migrations, profiles P2 Medium — degraded but workaround exists duplicate This issue or pull request already exists labels Jun 17, 2026
@alt-glitch

Copy link
Copy Markdown
Collaborator

Duplicate of #31468 — same bug and same fix: Feishu _apply_env_overrides() unconditionally forces enabled = True when FEISHU_APP_ID/FEISHU_APP_SECRET are present, overriding an explicit enabled: false. #31468 (open, earlier) already separates credential availability from listener enablement. Marking duplicate; this PR adds useful regression tests reviewers may want to salvage.

@liuhao1024

Copy link
Copy Markdown
Contributor Author

@alt-glitch This is not a duplicate of #31468. The two PRs fix the same symptom but use different approaches:

Both can coexist: #31468 adds a kill-switch env var, #47807 ensures config.yaml is respected. If only one lands, #47807 is the more minimal fix (zero new config surface).

@teknium1

Copy link
Copy Markdown
Contributor

Thanks for the focused fix. The premise is confirmed on current main: gateway/config.py:1876 still force-enables Feishu after reading its credentials, while _enable_from_env() already preserves an explicit disable at gateway/config.py:1465-1467.

Problems

  • The new tests exercise _apply_env_overrides() with a manually constructed _enabled_explicit marker. They do not exercise the real config-resolution path that creates the marker in load_gateway_config() (gateway/config.py:1126) and removes it after processing (gateway/config.py:2293).

Suggested changes

  • Add a temp-HERMES_HOME load_gateway_config() regression test using platforms.feishu.enabled: false plus Feishu credential environment variables, asserting Feishu remains disabled and the credentials are still available in extra.

This is an automated hermes-sweeper review.

@alt-glitch alt-glitch added sweeper:risk-compatibility Sweeper risk: may break existing users, config, migrations, defaults, or upgrades and removed duplicate This issue or pull request already exists labels Jul 14, 2026
@teknium1 teknium1 added the sweeper:blast-contained Sweeper blast radius: contained — one narrow path / opt-in / few users label Jul 14, 2026
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-contained Sweeper blast radius: contained — one narrow path / opt-in / few users 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.

[BUG] Feishu platform forced enabled by env vars, ignores config.yaml enabled: false

3 participants