From 6ab79fc147fd2a5c52ea63bf9c1bb7b1ebd3338a Mon Sep 17 00:00:00 2001 From: ygd58 Date: Tue, 21 Jul 2026 14:49:29 +0000 Subject: [PATCH 1/2] fix(plugins): warn when required env vars are missing, before register() Ports #63050 forward onto current main per teknium1's review. _load_plugin() emitted only a DEBUG registration count, so a plugin whose required env vars were absent loaded silently (issue #2768). - Normalize requires_env entries (str or dict, matching hermes_cli/plugins_cmd.py:307-314) before checking the process environment. - Emit a WARNING when any required env var is not set, naming each missing variable. - The empty-registration note stays DEBUG (not WARNING) to avoid false positives for provider-only plugins (e.g. image_gen/fal), whose registrations aren't tracked in LoadedPlugin's tools/hooks/middleware/ commands counts. Per review, fixes two real bugs in the original port: 1. The missing-env check ran AFTER register_fn(ctx). A plugin that reads the missing variable directly during registration raises straight into the generic 'Failed to load plugin' handler before the specific missing-variable diagnostic is ever reported. Moved the check before register_fn(ctx) so the WARNING fires regardless of whether registration itself succeeds. 2. The provider-only regression test used a no-op register(), which doesn't actually exercise a provider registration. Replaced with a real ImageGenProvider subclass registered via ctx.register_image_gen_provider(), matching how plugins/image_gen/fal actually registers. Scope note: this fix is in the general PluginManager loader (hermes_cli/plugins.py). The Hindsight memory plugin path uses a separate loader (plugins/memory/__init__.py) that general discovery skips entirely -- this PR does not touch that path. Added a raising-register() regression test per review, and the corrected real-provider test. 5/5 new tests pass; 121/121 in the full tests/hermes_cli/test_plugins.py file. --- hermes_cli/plugins.py | 61 ++++++++++- tests/hermes_cli/test_plugins.py | 180 +++++++++++++++++++++++++++++++ 2 files changed, 237 insertions(+), 4 deletions(-) diff --git a/hermes_cli/plugins.py b/hermes_cli/plugins.py index 6ca393fca53c..c759b6f04418 100644 --- a/hermes_cli/plugins.py +++ b/hermes_cli/plugins.py @@ -1775,6 +1775,36 @@ def _load_plugin(self, manifest: PluginManifest) -> None: logger.warning("Plugin '%s' has no register() function", manifest.name) else: ctx = PluginContext(manifest, self) + + # Warn when a declared required env var is missing, BEFORE + # calling register(). Some plugins read the variable + # directly during registration and raise (e.g. KeyError / + # a provider SDK's own validation) -- if this check ran + # only after register_fn(ctx) succeeded, that raise would + # propagate straight to the generic "Failed to load + # plugin" handler below and the specific missing-variable + # diagnostic would never be reported (issue #2768). + # Normalize requires_env entries (str or dict) before + # checking the process environment, mirroring + # hermes_cli/plugins_cmd.py:307-314. + _missing_env: list[str] = [] + for _entry in manifest.requires_env or []: + if isinstance(_entry, str): + _var_name = _entry + elif isinstance(_entry, dict): + _var_name = _entry.get("name", "") + else: + _var_name = "" + if _var_name and not os.environ.get(_var_name): + _missing_env.append(_var_name) + + if _missing_env: + logger.warning( + "Plugin '%s': required env var(s) not set: %s " + "-- plugin may be non-functional or fail to load", + manifest.name, ", ".join(_missing_env), + ) + # Snapshot registry state BEFORE register() so each registry's # attribution counts only what THIS plugin actually added. # The previous approach diffed names against all already-loaded @@ -1809,16 +1839,39 @@ def _load_plugin(self, manifest: PluginManifest) -> None: if self._plugin_commands[c].get("plugin") == manifest.name ] loaded.enabled = True + + _cli_cmd_count = sum( + 1 for c in self._cli_commands + if self._cli_commands[c].get("plugin") == manifest.name + ) + # A plugin that registered nothing on any tracked surface may + # still be legitimate: provider-only plugins (e.g. an image + # generation backend) register a provider, which isn't + # tracked in LoadedPlugin's tools/hooks/middleware/commands + # counts. Demoted to DEBUG (not WARNING) to avoid false + # positives for that case. + _nothing_registered = not any([ + loaded.tools_registered, + loaded.hooks_registered, + loaded.middleware_registered, + loaded.commands_registered, + _cli_cmd_count, + ]) + if _nothing_registered: + logger.debug( + "Plugin '%s' registered no tools, hooks, middleware, " + "or commands -- it may register a provider or be a " + "no-op stub", + manifest.name, + ) + logger.debug( " registered: %d tool(s), %d hook(s), %d middleware, %d slash command(s), %d CLI command(s)", len(loaded.tools_registered), len(loaded.hooks_registered), len(loaded.middleware_registered), len(loaded.commands_registered), - sum( - 1 for c in self._cli_commands - if self._cli_commands[c].get("plugin") == manifest.name - ), + _cli_cmd_count, ) except Exception as exc: diff --git a/tests/hermes_cli/test_plugins.py b/tests/hermes_cli/test_plugins.py index 3fdb6d1812d2..68a8c9ffc881 100644 --- a/tests/hermes_cli/test_plugins.py +++ b/tests/hermes_cli/test_plugins.py @@ -2366,3 +2366,183 @@ def test_dispatch_tool_invokes_handler_without_cli_ref(self): assert calls[0][1].get("parent_agent") is None finally: registry.deregister("_test_dispatch_probe") + + +class TestRequiresEnvWarning: + """Regression tests for issue #2768: _load_plugin() warns when required + env vars are missing, handling both str and dict manifest entries, and + warns BEFORE calling register() so a plugin that crashes because of the + missing variable still gets the specific diagnostic. + """ + + def test_requires_env_str_missing_emits_warning(self, tmp_path, monkeypatch, caplog): + """A missing env var declared as a plain string must produce a WARNING.""" + import logging + monkeypatch.setenv("HERMES_HOME", str(tmp_path)) + monkeypatch.delenv("_TEST_MISSING_VAR_2768", raising=False) + + plugin_dir = tmp_path / "plugins" / "envtest" + plugin_dir.mkdir(parents=True) + (plugin_dir / "__init__.py").write_text("def register(ctx): pass\n") + + mgr = PluginManager() + manifest = PluginManifest( + name="envtest", + source="user", + path=str(plugin_dir), + requires_env=["_TEST_MISSING_VAR_2768"], + ) + + with caplog.at_level(logging.WARNING, logger="hermes_cli.plugins"): + mgr._load_plugin(manifest) + + assert any( + "_TEST_MISSING_VAR_2768" in r.message + for r in caplog.records + if r.levelno == logging.WARNING + ), "Expected WARNING about missing env var, got: " + str([r.message for r in caplog.records]) + + def test_requires_env_dict_missing_emits_warning(self, tmp_path, monkeypatch, caplog): + """A missing env var declared as a dict {name: ...} must also warn.""" + import logging + monkeypatch.setenv("HERMES_HOME", str(tmp_path)) + monkeypatch.delenv("_TEST_DICT_VAR_2768", raising=False) + + plugin_dir = tmp_path / "plugins" / "envtest_dict" + plugin_dir.mkdir(parents=True) + (plugin_dir / "__init__.py").write_text("def register(ctx): pass\n") + + mgr = PluginManager() + manifest = PluginManifest( + name="envtest_dict", + source="user", + path=str(plugin_dir), + requires_env=[{"name": "_TEST_DICT_VAR_2768", "description": "test var"}], + ) + + with caplog.at_level(logging.WARNING, logger="hermes_cli.plugins"): + mgr._load_plugin(manifest) + + assert any( + "_TEST_DICT_VAR_2768" in r.message + for r in caplog.records + if r.levelno == logging.WARNING + ) + + def test_requires_env_present_no_warning(self, tmp_path, monkeypatch, caplog): + """When the required env var IS set, no missing-env warning must fire.""" + import logging + monkeypatch.setenv("HERMES_HOME", str(tmp_path)) + monkeypatch.setenv("_TEST_PRESENT_VAR_2768", "set") + + plugin_dir = tmp_path / "plugins" / "envtest_ok" + plugin_dir.mkdir(parents=True) + (plugin_dir / "__init__.py").write_text("def register(ctx): pass\n") + + mgr = PluginManager() + manifest = PluginManifest( + name="envtest_ok", + source="user", + path=str(plugin_dir), + requires_env=["_TEST_PRESENT_VAR_2768"], + ) + + with caplog.at_level(logging.WARNING, logger="hermes_cli.plugins"): + mgr._load_plugin(manifest) + + assert not any( + "_TEST_PRESENT_VAR_2768" in r.message and r.levelno == logging.WARNING + for r in caplog.records + ) + + def test_requires_env_warning_fires_even_when_register_raises(self, tmp_path, monkeypatch, caplog): + """Regression (review of #63050): a plugin that reads the missing + variable directly during registration and raises must still produce + the specific missing-env WARNING -- not just the generic "Failed to + load plugin" message from the outer exception handler. This is why + the check must run BEFORE register_fn(ctx), not after.""" + import logging + monkeypatch.setenv("HERMES_HOME", str(tmp_path)) + monkeypatch.delenv("_TEST_RAISING_VAR_2768", raising=False) + + plugin_dir = tmp_path / "plugins" / "envtest_raises" + plugin_dir.mkdir(parents=True) + (plugin_dir / "__init__.py").write_text( + "import os\n" + "def register(ctx):\n" + " os.environ['_TEST_RAISING_VAR_2768'] # KeyError: not set\n" + ) + + mgr = PluginManager() + manifest = PluginManifest( + name="envtest_raises", + source="user", + path=str(plugin_dir), + requires_env=["_TEST_RAISING_VAR_2768"], + ) + + with caplog.at_level(logging.WARNING, logger="hermes_cli.plugins"): + mgr._load_plugin(manifest) # must not raise -- outer handler catches it + + assert any( + "_TEST_RAISING_VAR_2768" in r.message + for r in caplog.records + if r.levelno == logging.WARNING + ), ( + "The specific missing-env-var WARNING must fire even though " + "register() raised: " + str([r.message for r in caplog.records]) + ) + # The plugin should also be marked failed via the generic handler. + loaded = mgr._plugins.get("envtest_raises") + assert loaded is not None and loaded.error, "Plugin load must still be marked failed" + + def test_provider_only_plugin_no_false_positive(self, tmp_path, monkeypatch, caplog): + """A plugin that registers a REAL image_gen provider (no tools/hooks/ + commands) must not produce a spurious empty-registration warning -- + provider registrations are not tracked in LoadedPlugin's + tools/hooks/middleware/commands counts.""" + import logging + monkeypatch.setenv("HERMES_HOME", str(tmp_path)) + + plugin_dir = tmp_path / "plugins" / "provider_only" + plugin_dir.mkdir(parents=True) + # A real provider registration (not a no-op), matching how + # plugins/image_gen/fal actually registers. + (plugin_dir / "__init__.py").write_text( + "from agent.image_gen_provider import ImageGenProvider\n" + "\n" + "class _TestProvider(ImageGenProvider):\n" + " @property\n" + " def name(self):\n" + " return 'test-provider-2768'\n" + " def generate(self, prompt, aspect_ratio='1:1', **kwargs):\n" + " return {}\n" + "\n" + "def register(ctx):\n" + " ctx.register_image_gen_provider(_TestProvider())\n" + ) + + mgr = PluginManager() + manifest = PluginManifest( + name="provider_only", + source="user", + path=str(plugin_dir), + requires_env=[], + ) + + try: + with caplog.at_level(logging.WARNING, logger="hermes_cli.plugins"): + mgr._load_plugin(manifest) + + warning_msgs = [r.message for r in caplog.records if r.levelno == logging.WARNING] + assert not any("registered no tools" in m for m in warning_msgs), ( + f"False positive warning for provider-only plugin: {warning_msgs}" + ) + loaded = mgr._plugins.get("provider_only") + assert loaded is not None and loaded.enabled, ( + f"Real provider registration must succeed, not error: " + f"{loaded.error if loaded else 'not loaded'}" + ) + finally: + from agent.image_gen_registry import _providers as _img_providers + _img_providers.pop("test-provider-2768", None) From 9b2896d193a0a073c39718d06c051b5c27c54220 Mon Sep 17 00:00:00 2001 From: ygd58 Date: Thu, 30 Jul 2026 05:09:04 +0000 Subject: [PATCH 2/2] fix(plugins): move missing-env check before module loading, verify real provider registration Follow-up per review of #68703. Two real gaps: 1. The missing-env check still ran after _load_directory_module()/_load_entrypoint_module() -- only before register_fn(ctx). A plugin that accesses a declared requires_env variable at MODULE scope (not just inside register()) still raises during the import itself, propagating straight to the generic "Failed to load plugin" handler before the specific diagnostic ever runs. Moved the check to run first, using manifest.requires_env directly (available before the module is ever imported, so no ordering dependency on the module existing at all). 2. The provider-only regression test's caplog capture only recorded WARNING records, while the empty-registration message it was guarding against is DEBUG -- so the WARNING-absence assertion never actually exercised anything, and loaded.enabled only proves register() returned without raising, not that the registration call inside it took effect. Added an assertion that the provider is actually retrievable via agent.image_gen_registry.get_provider(), which only succeeds if ctx.register_image_gen_provider() genuinely ran. Added the requested module-import-failure regression test: a plugin that raises accessing the missing variable at module scope (before register() is even an attribute on the module) must still produce the specific WARNING, proving the check now runs early enough to catch that case too. 122/122 tests pass in the full tests/hermes_cli/test_plugins.py file (2 new/fixed, no regression). --- hermes_cli/plugins.py | 60 +++++++++++++++++--------------- tests/hermes_cli/test_plugins.py | 58 ++++++++++++++++++++++++++++++ 2 files changed, 89 insertions(+), 29 deletions(-) diff --git a/hermes_cli/plugins.py b/hermes_cli/plugins.py index c759b6f04418..54023f26b4c8 100644 --- a/hermes_cli/plugins.py +++ b/hermes_cli/plugins.py @@ -1761,6 +1761,37 @@ def _load_plugin(self, manifest: PluginManifest) -> None: PluginContext(manifest, self)._tool_override_allowed(""), ) try: + # Warn when a declared required env var is missing, BEFORE + # loading the module at all. Some plugins read the variable + # directly at module scope (not just inside register()) and + # raise (e.g. KeyError / a provider SDK's own validation) -- + # if this check ran only after the module import (or only + # after register_fn(ctx) succeeded), that raise would + # propagate straight to the generic "Failed to load plugin" + # handler below and the specific missing-variable diagnostic + # would never be reported (issue #2768). Uses manifest.requires_env + # directly since it's available before the module is ever + # imported. Normalize requires_env entries (str or dict) before + # checking the process environment, mirroring + # hermes_cli/plugins_cmd.py:307-314. + _missing_env: list[str] = [] + for _entry in manifest.requires_env or []: + if isinstance(_entry, str): + _var_name = _entry + elif isinstance(_entry, dict): + _var_name = _entry.get("name", "") + else: + _var_name = "" + if _var_name and not os.environ.get(_var_name): + _missing_env.append(_var_name) + + if _missing_env: + logger.warning( + "Plugin '%s': required env var(s) not set: %s " + "-- plugin may be non-functional or fail to load", + manifest.name, ", ".join(_missing_env), + ) + if manifest.source in {"user", "project", "bundled"}: module = self._load_directory_module(manifest) else: @@ -1776,35 +1807,6 @@ def _load_plugin(self, manifest: PluginManifest) -> None: else: ctx = PluginContext(manifest, self) - # Warn when a declared required env var is missing, BEFORE - # calling register(). Some plugins read the variable - # directly during registration and raise (e.g. KeyError / - # a provider SDK's own validation) -- if this check ran - # only after register_fn(ctx) succeeded, that raise would - # propagate straight to the generic "Failed to load - # plugin" handler below and the specific missing-variable - # diagnostic would never be reported (issue #2768). - # Normalize requires_env entries (str or dict) before - # checking the process environment, mirroring - # hermes_cli/plugins_cmd.py:307-314. - _missing_env: list[str] = [] - for _entry in manifest.requires_env or []: - if isinstance(_entry, str): - _var_name = _entry - elif isinstance(_entry, dict): - _var_name = _entry.get("name", "") - else: - _var_name = "" - if _var_name and not os.environ.get(_var_name): - _missing_env.append(_var_name) - - if _missing_env: - logger.warning( - "Plugin '%s': required env var(s) not set: %s " - "-- plugin may be non-functional or fail to load", - manifest.name, ", ".join(_missing_env), - ) - # Snapshot registry state BEFORE register() so each registry's # attribution counts only what THIS plugin actually added. # The previous approach diffed names against all already-loaded diff --git a/tests/hermes_cli/test_plugins.py b/tests/hermes_cli/test_plugins.py index 68a8c9ffc881..525751a64d59 100644 --- a/tests/hermes_cli/test_plugins.py +++ b/tests/hermes_cli/test_plugins.py @@ -2496,6 +2496,56 @@ def test_requires_env_warning_fires_even_when_register_raises(self, tmp_path, mo loaded = mgr._plugins.get("envtest_raises") assert loaded is not None and loaded.error, "Plugin load must still be marked failed" + def test_requires_env_warning_fires_even_when_module_import_raises( + self, tmp_path, monkeypatch, caplog + ): + """Regression (review of #68703): a plugin that reads the missing + variable at MODULE scope -- during import itself, before + register() is even reachable as an attribute -- must still produce + the specific missing-env WARNING. The prior fix moved the check + before register_fn(ctx) but still after + _load_directory_module()/_load_entrypoint_module(), so a raise + during the import itself still bypassed the diagnostic. The check + must run before module loading, using manifest.requires_env + directly rather than anything read off the imported module.""" + import logging + monkeypatch.setenv("HERMES_HOME", str(tmp_path)) + monkeypatch.delenv("_TEST_IMPORT_RAISING_VAR_2768", raising=False) + + plugin_dir = tmp_path / "plugins" / "envtest_import_raises" + plugin_dir.mkdir(parents=True) + (plugin_dir / "__init__.py").write_text( + "import os\n" + "# Accessed at MODULE scope -- raises during the import itself,\n" + "# before register() is even an attribute on the module yet.\n" + "_REQUIRED = os.environ['_TEST_IMPORT_RAISING_VAR_2768']\n" + "def register(ctx):\n" + " pass\n" + ) + + mgr = PluginManager() + manifest = PluginManifest( + name="envtest_import_raises", + source="user", + path=str(plugin_dir), + requires_env=["_TEST_IMPORT_RAISING_VAR_2768"], + ) + + with caplog.at_level(logging.WARNING, logger="hermes_cli.plugins"): + mgr._load_plugin(manifest) # must not raise -- outer handler catches it + + assert any( + "_TEST_IMPORT_RAISING_VAR_2768" in r.message + for r in caplog.records + if r.levelno == logging.WARNING + ), ( + "The specific missing-env-var WARNING must fire even though " + "the module import itself raised: " + + str([r.message for r in caplog.records]) + ) + loaded = mgr._plugins.get("envtest_import_raises") + assert loaded is not None and loaded.error, "Plugin load must still be marked failed" + def test_provider_only_plugin_no_false_positive(self, tmp_path, monkeypatch, caplog): """A plugin that registers a REAL image_gen provider (no tools/hooks/ commands) must not produce a spurious empty-registration warning -- @@ -2543,6 +2593,14 @@ def test_provider_only_plugin_no_false_positive(self, tmp_path, monkeypatch, cap f"Real provider registration must succeed, not error: " f"{loaded.error if loaded else 'not loaded'}" ) + from agent.image_gen_registry import get_provider as _get_img_provider + registered = _get_img_provider("test-provider-2768") + assert registered is not None, ( + "The provider must actually be retrievable from " + "agent.image_gen_registry -- loaded.enabled alone only " + "proves register() returned without raising, not that " + "the registration call inside it actually took effect" + ) finally: from agent.image_gen_registry import _providers as _img_providers _img_providers.pop("test-provider-2768", None)