fix(gateway): respect explicit enabled:false in YAML for all platforms - #38745
fix(gateway): respect explicit enabled:false in YAML for all platforms#38745porkmagus wants to merge 2 commits into
Conversation
When a user disables a platform in config.yaml (or via hermes setup gateway), the corresponding env vars remain in ~/.hermes/.env. On the next gateway restart, _apply_env_overrides() sees those env vars and re-enables the platform, ignoring the explicit disable. This fix: 1. Promotes _enabled_explicit from extra dict (consumed on load) to a real PlatformConfig dataclass field that persists across restarts. 2. Replaces _enable_from_env() with _maybe_enable_from_env() that only auto-enables when no YAML config exists for that platform. 3. Applies the fix to ALL platforms (not just Telegram/Discord), including HomeAssistant, Email, SMS, DingTalk, Feishu, WeCom, WeCom callback, Weixin, BlueBubbles, QQ, Yuanbao, API Server, Webhook, MSGraph Webhook. 4. Adds hermes env CLI suite (path, list, get, set, rm, edit) for discoverability of .env. 5. Adds automatic .env cleanup on disable via env_cleanup.py module. 6. Hooks cleanup into hermes config set, hermes tools disable, and gateway setup wizard. Fixes #35555, #37609 Refs PR #35562 (partial fix for Telegram/Discord only) Refs PR #18764 (merged May 2 - same pattern for agent.max_turns)
There was a problem hiding this comment.
Pull request overview
Note
Copilot was unable to run its full agentic suite in this review.
Adds CLI support and safety mechanisms to prevent disabled platforms/tools from being silently re-enabled by leftover .env variables, and introduces an env subcommand for managing ~/.hermes/.env.
Changes:
- Add env-var cleanup mappings and cleanup hooks when disabling platforms/tools.
- Introduce
hermes env ...CLI subcommands (path/list/get/set/rm/edit) implemented inhermes_cli.config. - Make gateway env overrides respect an explicit
enabledchoice via a persistentenabled_explicitflag.
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 9 comments.
Show a summary per file
| File | Description |
|---|---|
| hermes_cli/tools_config.py | Removes tool-related env vars when toolsets are disabled. |
| hermes_cli/main.py | Wires a new env CLI command and subcommands into the main parser. |
| hermes_cli/env_cleanup.py | New central mapping + helper to remove env vars for platforms/tools. |
| hermes_cli/config.py | Cleans platform env vars on explicit disable; implements env_command handler. |
| gateway/config.py | Persists enabled_explicit and prevents env vars from re-enabling explicitly-disabled platforms. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| # Clean up .env vars when a platform is explicitly disabled | ||
| if key.startswith("platforms.") and key.endswith(".enabled") and value is False: | ||
| platform_name = key.split(".")[1] | ||
| from hermes_cli.env_cleanup import remove_env_vars_for_feature | ||
| removed = remove_env_vars_for_feature("platform", platform_name) | ||
| if removed: | ||
| logger.info("Removed %s env vars for disabled platform %s", len(removed), platform_name) |
There was a problem hiding this comment.
Fixed: Changed type to Union[str, bool, int, float] and added isinstance(value, bool) guard.
| if value is not None: | ||
| # Mask secrets | ||
| if len(value) > 8: | ||
| print(f"{key}={value[:4]}{'*' * (len(value) - 8)}{value[-4:]}") | ||
| else: | ||
| print(f"{key}={value}") |
There was a problem hiding this comment.
Fixed: Always mask secrets. Short values (≤4 chars) show all asterisks; longer values show first/last 2 chars.
| editor = os.environ.get("EDITOR", "nano") | ||
| env_path = get_env_path() |
There was a problem hiding this comment.
Fixed: Uses shlex.split() with fallback to raw string for $EDITOR with arguments/spaces.
| if not env_path.exists(): | ||
| env_path.write_text("# Hermes environment variables\n", encoding="utf-8") | ||
| subprocess.run([editor, str(env_path)]) |
There was a problem hiding this comment.
Fixed: Uses shlex.split() with fallback to raw string for $EDITOR with arguments/spaces.
| } | ||
|
|
||
|
|
||
| def remove_env_vars_for_feature(category: str, name: str) -> List[str]: |
There was a problem hiding this comment.
Fixed: Added ValueError if category is not 'platform' or 'tool'.
| from hermes_cli.config import remove_env_value | ||
|
|
||
| removed: List[str] = [] | ||
| mapping = PLATFORM_ENV_VARS if category == "platform" else TOOL_ENV_VARS | ||
| for key in mapping.get(name, []): | ||
| if remove_env_value(key): | ||
| removed.append(key) | ||
| return removed |
There was a problem hiding this comment.
Fixed: Added ValueError if category is not 'platform' or 'tool'.
| # env list | ||
| env_subparsers.add_parser("list", help="List all env vars (values masked)") |
There was a problem hiding this comment.
Fixed: Updated help text to 'List env var names' to match actual behavior.
| # Clean up .env vars for disabled tools | ||
| for tool_name in toolset_names: | ||
| from hermes_cli.env_cleanup import remove_env_vars_for_feature | ||
| removed = remove_env_vars_for_feature("tool", tool_name) |
There was a problem hiding this comment.
Fixed: Moved import to top of function.
| # WhatsApp (typically uses different auth mechanism) | ||
| whatsapp_enabled = os.getenv("WHATSAPP_ENABLED", "").lower() in {"true", "1", "yes"} | ||
| whatsapp_disabled_explicitly = os.getenv("WHATSAPP_ENABLED", "").lower() in {"false", "0", "no"} | ||
| if Platform.WHATSAPP in config.platforms: | ||
| # YAML config exists — respect explicit disable | ||
| # YAML config exists — respect explicit disable. Only override if the | ||
| # user never explicitly set enabled (legacy env-only migration). | ||
| wa_cfg = config.platforms[Platform.WHATSAPP] | ||
| if whatsapp_disabled_explicitly: | ||
| wa_cfg.enabled = False | ||
| elif whatsapp_enabled: | ||
| if whatsapp_enabled and not wa_cfg.enabled and not wa_cfg.enabled_explicit: | ||
| wa_cfg.enabled = True |
There was a problem hiding this comment.
Fixed: Added back whatsapp_disabled_explicitly with enabled_explicit gate. YAML wins when explicitly set; env wins only for legacy/unconfigured setups.
…n, WhatsApp semantics
|
I found one issue that looks worth fixing before merge.
# Line 328:
if key.startswith("platforms.") and key.endswith(".enabled") and value is False:
Suggested fix: if key.startswith("platforms.") and key.endswith(".enabled") and value.lower() in ("false", "0", "no"):Why it matters: The whole point of the |
|
Fixed in commit 810a6c9. Addressed all 9 Copilot review items and the bug reported by @liuhao1024:
All 4 modified files pass . |
|
Fixed in commit 810a6c9. Addressed all 9 Copilot review items and the
All 4 modified files pass |
|
@liuhao1024 Fixed the bug you found in commit 810a6c9. Changed type to and added guard. The cleanup now triggers correctly when ✓ Set platforms.telegram.enabled = False in /Users/sean/.hermes/config.yaml is called. Thanks for the catch. |
|
@liuhao1024 Fixed the |
Fix:
.envenv vars silently override explicitplatforms.*.enabled: falseProblem
When a user disables a platform in
config.yaml(or viahermes setup gateway), the corresponding env vars remain in~/.hermes/.env. On the next gateway restart,_apply_env_overrides()sees those env vars and re-enables the platform, ignoring the explicit disable.This affects all platforms. The pattern is identical for tools: env vars in
.envkeep tools active even afterhermes tools disable <name>.Related Issues
Profile config ignored when TELEGRAM_BOT_TOKEN is in environment(same root cause)Desktop Skills & Tools toggle silently fails for platform-restricted toolsets(tool variant of config split).envcleanup, no CLIagent.max_turns/display/timezone(May 2).envpinningHERMES_DASHBOARD_SESSION_TOKENcauses boot loopReproduction
hermes setup gateway→ enable Slack → savesSLACK_BOT_TOKENto.envandplatforms.slack.enabled: truetoconfig.yamlhermes config set platforms.slack.enabled false(or uncheck in setup wizard)config.yamlnow saysenabled: falseSLACK_BOT_TOKENis in.envWHATSAPP_ENABLED=true), Signal (SIGNAL_HTTP_URL), etc.Root Cause
gateway/config.py::_apply_env_overrides()treats the presence of an env var as implicit activation intent:The
_enabled_explicitflag inextradict was supposed to prevent this, but:hermes config setFor WhatsApp, it is even worse:
WHATSAPP_ENABLEDenv var unconditionally setsenabled = Truewithout checking the existing config at all.For HomeAssistant, Email, SMS, DingTalk, Feishu, WeCom, WeCom callback, Weixin, BlueBubbles, QQ, Yuanbao, API Server, Webhook, and MSGraph Webhook — the same unconditional
enabled = Truepattern exists with no guard at all.User Impact
.envfile.env(hermes config env-pathonly prints the path).envpoison behind on disable~/.hermes/.envexists, then use a text editor to remove variablesSolution
1.
config.yamlwins forenabledstate (all platforms)Change
_apply_env_overrides()to only auto-enable a platform when no YAML config exists for that platform. If the user has ever configured the platform, the env var is treated as a credential only, not an activation signal.Apply this to all platforms currently using the broken pattern:
For WhatsApp, the
WHATSAPP_ENABLEDenv var must stop overriding the YAMLenabledflag. It should only be read as a fallback when no YAML config exists.2. Promote
enabled_explicitto a realPlatformConfigfieldThe current
_enabled_explicitis stored inextradict and consumed on load. Promote it to a properboolfield on the dataclass:from_dict()setsenabled_explicit = Truewheneverenabledis present in YAMLhermes config setsetsenabled_explicit = Truewhen it modifiesenabled_apply_env_overrides()checksenabled_explicitbefore auto-enabling3. Add
hermes envCLI suiteUsers currently have no way to discover or edit
.envfrom the CLI.The
editcommand shows a warning before opening:4. Automatic
.envcleanup on disableWhen a user disables a platform or tool, remove the corresponding env vars from
.envimmediately.hermes_cli/config.pyalready hasremove_env_value()— we need to call it.Platform mapping (derived from actual
_apply_env_overridesand platform code):Tool mapping (derived from
TOOL_CATEGORIESandTOOLSET_ENV_REQUIREMENTS):Hook this into:
hermes config set platforms.<name>.enabled false→ removes env varshermes setup gatewayuncheck → removes env varshermes tools disable <name>→ removes env vars5. Single source of truth for env→feature mapping
Create a single module that maps features to their env vars:
Import this in
hermes_cli/config.py(forconfig set),hermes_cli/gateway.py(for setup wizard), andhermes_cli/tools_config.py(fortools disable).6. Backwards compatibility
enabled: falsein YAML but env vars still present: on next load, the platform stays disabled (fixed behavior). The env vars remain in.envbut are ignored. A one-time warning is logged on first restart: "Platform X is disabled in config.yaml but env vars remain in .env. Runhermes env rm X_TOKENto clean up, or re-enable the platform to use them."Files to Modify
gateway/config.py_apply_env_overrides()for all platforms; addenabled_explicit: booltoPlatformConfighermes_cli/config.pyenvsubcommand suite; callremove_env_vars_for_feature()onplatforms.*.enabled falsehermes_cli/env_cleanup.pyhermes_cli/gateway.pyremove_env_vars_for_feature()on platform disable in setup wizardhermes_cli/tools_config.pyremove_env_vars_for_feature()ontools disablehermes_cli/main.pyenvcommand grouptests/gateway/test_config.pytests/cli/test_env.pyhermes envcommandstests/cli/test_env_cleanup.pyTest Plan
Regression Tests
CLI Tests
Cleanup Tests
Migration / Rollout
enabled: false+ env vars will see a log warning on first restart after upgrade. The platform stays disabled (correct behavior), but they are informed about orphaned env vars.hermes env cleanupsubcommand can be added later to remove all env vars for disabled features in one pass.Design Notes
Why not remove
.enventirely?.envis still needed for Docker deployments, CI/CD pipelines, and users who prefer env-only configuration. The goal is not to eliminate.env, but to stop it from silently overridingconfig.yaml.Why not merge PR #35562 instead?
PR #35562 is a good partial fix for Telegram/Discord. This PR:
.envcleanup mechanismhermes envCLI for discoverabilityThese are complementary. If #35562 merges first, this PR can be rebased to add the remaining platforms + CLI + cleanup.
Why
enabled_explicitas a dataclass field?The current
extradict approach is fragile: untyped, consumed on load, not visible in the schema, and not set byhermes config set. A real field solves all of these and makes the intent explicit in the code.Checklist