refactor: fold simplify-code review findings
- matrix/dingtalk: extract deps-only installers (ensure_matrix_deps, ensure_dingtalk_deps) and register THOSE as ensure_deps_fn — the prior check_*_requirements combined credential env checks with the install, so a platform configured via PlatformConfig.extra (which is_connected accepts) would pass enablement, reach create_adapter(), and have the 'installer' veto on env-var grounds before installing anything — re-creating the #79812 deadlock for extra-configured setups. The combined deps+credentials functions remain for setup/status callers. - matrix/feishu passive probes: use the existing lazy_deps.is_available() instead of hand-rolling 'not feature_missing(...)' (reuse finding). - teams: module docstring no longer recommends bare system pip (the PEP 668 trap purged everywhere else); docs troubleshooting row updated to match the new hint text. - wecom_callback: drop dead 'global ET, DEFUSEDXML_AVAILABLE' (ensure_and_bind mutates the module dict directly; nothing assigns). - tests: parametrized wiring contract for all 8 lazy-installable platforms — ensure_deps_fn present and distinct from check_fn (behavior contract, not identity snapshot, so renames don't churn it).
This commit is contained in:
@@ -174,34 +174,54 @@ def dingtalk_deps_present() -> bool:
|
||||
return DINGTALK_STREAM_AVAILABLE and HTTPX_AVAILABLE
|
||||
|
||||
|
||||
def check_dingtalk_requirements() -> bool:
|
||||
"""Check if DingTalk dependencies are available and configured.
|
||||
def ensure_dingtalk_deps() -> bool:
|
||||
"""ACTIVE deps-only installer (registry ``ensure_deps_fn``).
|
||||
|
||||
Lazy-installs dingtalk-stream via ``tools.lazy_deps.ensure("platform.dingtalk")``
|
||||
on first call if not present.
|
||||
Lazy-installs dingtalk-stream/httpx and rebinds module globals.
|
||||
Deliberately does NOT check credentials — ``ensure_deps_fn``'s contract
|
||||
is deps-only ("Returns True once deps are importable"); credentials are
|
||||
gated by ``is_connected``/``validate_config``. Otherwise a platform
|
||||
configured via ``PlatformConfig.extra`` (which ``_is_connected``
|
||||
accepts) would pass enablement, reach ``create_adapter()``, and have
|
||||
the installer veto on env-var grounds before ever installing —
|
||||
re-creating the #79812 deadlock for extra-configured setups.
|
||||
"""
|
||||
global DINGTALK_STREAM_AVAILABLE, dingtalk_stream, ChatbotMessage, CallbackMessage, AckMessage
|
||||
global HTTPX_AVAILABLE, httpx
|
||||
if not DINGTALK_STREAM_AVAILABLE or not HTTPX_AVAILABLE:
|
||||
try:
|
||||
from tools.lazy_deps import ensure as _lazy_ensure
|
||||
_lazy_ensure("platform.dingtalk", prompt=False)
|
||||
except Exception:
|
||||
return False
|
||||
try:
|
||||
import dingtalk_stream as _ds
|
||||
from dingtalk_stream import ChatbotMessage as _CM
|
||||
from dingtalk_stream.frames import CallbackMessage as _CBM, AckMessage as _AM
|
||||
import httpx as _httpx
|
||||
except Exception:
|
||||
return False
|
||||
dingtalk_stream = _ds
|
||||
ChatbotMessage = _CM
|
||||
CallbackMessage = _CBM
|
||||
AckMessage = _AM
|
||||
httpx = _httpx
|
||||
DINGTALK_STREAM_AVAILABLE = True
|
||||
HTTPX_AVAILABLE = True
|
||||
if DINGTALK_STREAM_AVAILABLE and HTTPX_AVAILABLE:
|
||||
return True
|
||||
try:
|
||||
from tools.lazy_deps import ensure as _lazy_ensure
|
||||
_lazy_ensure("platform.dingtalk", prompt=False)
|
||||
except Exception:
|
||||
return False
|
||||
try:
|
||||
import dingtalk_stream as _ds
|
||||
from dingtalk_stream import ChatbotMessage as _CM
|
||||
from dingtalk_stream.frames import CallbackMessage as _CBM, AckMessage as _AM
|
||||
import httpx as _httpx
|
||||
except Exception:
|
||||
return False
|
||||
dingtalk_stream = _ds
|
||||
ChatbotMessage = _CM
|
||||
CallbackMessage = _CBM
|
||||
AckMessage = _AM
|
||||
httpx = _httpx
|
||||
DINGTALK_STREAM_AVAILABLE = True
|
||||
HTTPX_AVAILABLE = True
|
||||
return True
|
||||
|
||||
|
||||
def check_dingtalk_requirements() -> bool:
|
||||
"""Check if DingTalk dependencies are available and configured.
|
||||
|
||||
Lazy-installs dingtalk-stream via :func:`ensure_dingtalk_deps`, then
|
||||
additionally requires credentials. Kept for setup/status callers that
|
||||
want the combined deps+credentials answer; the registry uses the
|
||||
deps-only :func:`ensure_dingtalk_deps` as ``ensure_deps_fn``.
|
||||
"""
|
||||
if not ensure_dingtalk_deps():
|
||||
return False
|
||||
if not os.getenv("DINGTALK_CLIENT_ID") or not _get_scoped_secret("DINGTALK_CLIENT_SECRET"):
|
||||
return False
|
||||
return True
|
||||
@@ -1894,7 +1914,7 @@ def register(ctx) -> None:
|
||||
label="DingTalk",
|
||||
adapter_factory=_build_adapter,
|
||||
check_fn=dingtalk_deps_present,
|
||||
ensure_deps_fn=check_dingtalk_requirements,
|
||||
ensure_deps_fn=ensure_dingtalk_deps,
|
||||
is_connected=_is_connected,
|
||||
validate_config=_is_connected,
|
||||
required_env=["DINGTALK_CLIENT_ID", "DINGTALK_CLIENT_SECRET"],
|
||||
|
||||
@@ -1451,7 +1451,7 @@ def feishu_deps_present() -> bool:
|
||||
"""PASSIVE probe: is lark-oapi installed right now?
|
||||
|
||||
Registry ``check_fn`` — called from status displays and config loading,
|
||||
so it must never install anything. Uses ``feature_missing`` (cheap
|
||||
so it must never install anything. Uses ``is_available`` (cheap
|
||||
importlib.metadata lookups) instead of importing the SDK, which is
|
||||
deferred to ``_load_lark_oapi`` at connect time. The ACTIVE
|
||||
lazy-installer (``check_feishu_requirements``) is registered as
|
||||
@@ -1461,8 +1461,8 @@ def feishu_deps_present() -> bool:
|
||||
if FEISHU_AVAILABLE:
|
||||
return True
|
||||
try:
|
||||
from tools.lazy_deps import feature_missing
|
||||
return not feature_missing("platform.feishu")
|
||||
from tools.lazy_deps import is_available
|
||||
return is_available("platform.feishu")
|
||||
except Exception: # pragma: no cover — defensive
|
||||
return False
|
||||
|
||||
|
||||
@@ -892,14 +892,13 @@ def matrix_deps_present() -> bool:
|
||||
"""PASSIVE probe: are the ``platform.matrix`` packages installed?
|
||||
|
||||
Registry ``check_fn`` — called from status displays and config loading,
|
||||
so it must never install anything. ``feature_missing`` is cheap
|
||||
(per-spec importlib.metadata lookups). The ACTIVE lazy-installer
|
||||
so it must never install anything. The ACTIVE lazy-installer
|
||||
(``check_matrix_requirements``) is registered as ``ensure_deps_fn``
|
||||
and runs from ``create_adapter()`` when this returns False (#79812).
|
||||
"""
|
||||
try:
|
||||
from tools.lazy_deps import feature_missing
|
||||
return not feature_missing("platform.matrix")
|
||||
from tools.lazy_deps import is_available
|
||||
return is_available("platform.matrix")
|
||||
except Exception: # pragma: no cover — defensive
|
||||
return False
|
||||
|
||||
@@ -907,13 +906,12 @@ def matrix_deps_present() -> bool:
|
||||
def check_matrix_requirements() -> bool:
|
||||
"""Return True if the Matrix adapter can be used.
|
||||
|
||||
Lazy-installs the full ``platform.matrix`` feature group via
|
||||
``tools.lazy_deps.ensure_and_bind`` whenever any of the declared
|
||||
packages (mautrix, Markdown, aiosqlite, asyncpg, aiohttp-socks) is
|
||||
missing — not just mautrix itself. Previously this short-circuited on
|
||||
``import mautrix``, which left the other four packages uninstalled
|
||||
forever and broke E2EE connect with ``No module named 'asyncpg'``
|
||||
(#31116). Rebinds module-level type globals on success.
|
||||
Combined credentials + deps answer for setup/status callers. The
|
||||
registry's ``ensure_deps_fn`` is the deps-only
|
||||
:func:`ensure_matrix_deps` below — credentials must NOT gate the
|
||||
installer (they're handled by ``is_connected``, which also accepts
|
||||
``PlatformConfig.extra``-configured setups that these env checks
|
||||
would wrongly veto).
|
||||
"""
|
||||
token = _startup_env_secret("MATRIX_ACCESS_TOKEN")
|
||||
password = _startup_env_secret("MATRIX_PASSWORD")
|
||||
@@ -926,6 +924,20 @@ def check_matrix_requirements() -> bool:
|
||||
logger.warning("Matrix: MATRIX_HOMESERVER not set")
|
||||
return False
|
||||
|
||||
return ensure_matrix_deps()
|
||||
|
||||
|
||||
def ensure_matrix_deps() -> bool:
|
||||
"""ACTIVE deps-only installer (registry ``ensure_deps_fn``).
|
||||
|
||||
Lazy-installs the full ``platform.matrix`` feature group via
|
||||
``tools.lazy_deps.ensure_and_bind`` whenever any of the declared
|
||||
packages (mautrix, Markdown, aiosqlite, asyncpg, aiohttp-socks) is
|
||||
missing — not just mautrix itself. Previously this short-circuited on
|
||||
``import mautrix``, which left the other four packages uninstalled
|
||||
forever and broke E2EE connect with ``No module named 'asyncpg'``
|
||||
(#31116). Rebinds module-level type globals on success.
|
||||
"""
|
||||
# Check whether any package in the platform.matrix feature group is
|
||||
# missing. ``feature_missing`` is cheap (per-spec importlib.metadata
|
||||
# lookups) and correctly handles ``mautrix[encryption]`` by stripping
|
||||
@@ -5293,7 +5305,7 @@ def register(ctx) -> None:
|
||||
label="Matrix",
|
||||
adapter_factory=_build_adapter,
|
||||
check_fn=matrix_deps_present,
|
||||
ensure_deps_fn=check_matrix_requirements,
|
||||
ensure_deps_fn=ensure_matrix_deps,
|
||||
is_connected=_is_connected,
|
||||
required_env=["MATRIX_HOMESERVER", "MATRIX_ACCESS_TOKEN"],
|
||||
install_hint="pip install 'mautrix[encryption]'",
|
||||
|
||||
@@ -6,7 +6,8 @@ Runs an aiohttp webhook server to receive messages from Teams.
|
||||
Proactive messaging (send, typing) uses the SDK's App.send() method.
|
||||
|
||||
Requires:
|
||||
pip install microsoft-teams-apps aiohttp
|
||||
the ``teams`` extra (auto-installed by the gateway on first start, or
|
||||
manually: ``<hermes-venv>/bin/pip install microsoft-teams-apps aiohttp``)
|
||||
TEAMS_CLIENT_ID, TEAMS_CLIENT_SECRET, and TEAMS_TENANT_ID env vars
|
||||
|
||||
Configuration in config.yaml:
|
||||
|
||||
@@ -88,7 +88,6 @@ def ensure_wecom_callback_requirements() -> bool:
|
||||
False forever on installs without the ``wecom`` extra and the
|
||||
``platform.wecom_callback`` LAZY_DEPS entry was never exercised.
|
||||
"""
|
||||
global ET, DEFUSEDXML_AVAILABLE
|
||||
if check_wecom_callback_requirements():
|
||||
return True
|
||||
|
||||
|
||||
@@ -656,3 +656,44 @@ class TestPluginEnablementGate:
|
||||
assert cfg.platforms[plat].enabled is False
|
||||
finally:
|
||||
_reg.unregister("myhardblockplat")
|
||||
|
||||
|
||||
class TestMigratedPlatformWiring:
|
||||
"""Every lazy-installable bundled platform must register the split:
|
||||
a PASSIVE check_fn plus an ACTIVE ensure_deps_fn (#79812).
|
||||
|
||||
Behavior contract, not a snapshot: asserts the two fields are distinct
|
||||
callables (probe != installer), not specific function identities, so
|
||||
renames don't churn this test.
|
||||
"""
|
||||
|
||||
import pytest as _pytest
|
||||
|
||||
@_pytest.mark.parametrize(
|
||||
"platform_name",
|
||||
[
|
||||
"teams", "telegram", "discord", "slack",
|
||||
"matrix", "dingtalk", "feishu", "wecom_callback",
|
||||
],
|
||||
)
|
||||
def test_lazy_installable_platform_has_split_wiring(self, platform_name):
|
||||
from hermes_cli.plugins import discover_plugins
|
||||
|
||||
discover_plugins()
|
||||
from gateway.platform_registry import platform_registry
|
||||
|
||||
# Materialize deferred loaders (wecom_callback is registered by the
|
||||
# "wecom" manifest's loader; a cold get() by its own name misses).
|
||||
platform_registry.plugin_entries()
|
||||
entry = platform_registry.get(platform_name)
|
||||
assert entry is not None, f"{platform_name} not registered"
|
||||
assert entry.ensure_deps_fn is not None, (
|
||||
f"{platform_name} has a lazy-installable SDK but no "
|
||||
"ensure_deps_fn — its deps can never auto-install "
|
||||
"(the #79812 deadlock)"
|
||||
)
|
||||
assert entry.ensure_deps_fn is not entry.check_fn, (
|
||||
f"{platform_name} registered the same callable for the passive "
|
||||
"probe and the active installer — status displays would "
|
||||
"pip-install as a side effect"
|
||||
)
|
||||
|
||||
@@ -255,7 +255,7 @@ Make sure your configured port (`TEAMS_PORT`, default `3978`) is reachable from
|
||||
| Problem | Solution |
|
||||
|---------|----------|
|
||||
| `Can't find a suitable configuration file` from `docker compose` | You are not in the repo that has `docker-compose.yml`, or you are on a native install — use `hermes gateway restart` instead, or `cd` into the clone first |
|
||||
| `requirements not met (pip install microsoft-teams-apps …)` / `No adapter available for teams` | Restart gateway so lazy-install can run, or install into the **Hermes venv**: `~/.hermes/hermes-agent/venv/bin/pip install microsoft-teams-apps aiohttp`. System `pip` fails on Ubuntu 24.04 (PEP 668) and would not affect the service anyway |
|
||||
| `requirements not met` / `Teams SDK missing` / `No adapter available for teams` | Restart gateway so lazy-install can run, or install into the **Hermes venv**: `~/.hermes/hermes-agent/venv/bin/pip install microsoft-teams-apps aiohttp`. System `pip` fails on Ubuntu 24.04 (PEP 668) and would not affect the service anyway |
|
||||
| `health` endpoint works but bot doesn't respond | Check that your tunnel is still running and the bot's messaging endpoint matches the tunnel URL |
|
||||
| `KeyError: 'teams'` in logs | Restart the container — this is fixed in the current version |
|
||||
| Bot responds with auth errors | Verify `TEAMS_CLIENT_ID`, `TEAMS_CLIENT_SECRET`, and `TEAMS_TENANT_ID` are all set correctly |
|
||||
|
||||
Reference in New Issue
Block a user