fix(plugins): warn when required env vars are missing, before register()

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:
@@ -1775,6 +1806,7 @@ def _load_plugin(self, manifest: PluginManifest) -> None:
                 logger.warning("Plugin '%s' has no register() function", manifest.name)
             else:
                 ctx = PluginContext(manifest, self)
+
                 # 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 +1841,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:

tests/hermes_cli/test_plugins.py

@@ -2366,3 +2366,241 @@ 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_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 --
+        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'}"
+            )
+            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)