fix(plugins): warn when required env vars are missing, handle str/dict manifest entries - #63050
fix(plugins): warn when required env vars are missing, handle str/dict manifest entries#63050ygd58 wants to merge 1 commit into
Conversation
…t manifest entries _load_plugin() emitted only a DEBUG registration count, so a plugin whose required env vars were absent loaded silently and produced confusing no-op behavior (issue NousResearch#2768). Fix: - Normalize requires_env entries (str or dict, matching the schema in hermes_cli/plugins_cmd.py:307-314) before checking the process environment, so both manifest forms are handled correctly. - Emit a WARNING when any required env var is not set, naming each missing variable so the operator can act on it. - The empty-registration note is demoted to DEBUG (not WARNING) to avoid false positives: provider-only plugins (e.g. image_gen/fal) register a provider rather than tools/hooks/commands, and those registrations are not tracked in LoadedPlugin; a WARNING there would be noise. Tests added to tests/hermes_cli/test_plugins.py: - str-form missing env var -> WARNING with var name - dict-form missing env var -> WARNING with var name - present env var -> no spurious WARNING - provider-only plugin (no tools/hooks) -> no false-positive WARNING 4/4 new tests pass alongside 100 existing plugin tests. Fixes NousResearch#2768
teknium1
left a comment
There was a problem hiding this comment.
Thanks for carrying forward the manifest normalization and regression coverage. The generic missing-env diagnostic is still needed on current main: hermes_cli/plugins.py:1792-1821 registers plugins and logs only DEBUG counts.
Problems
- The new missing-env check is after
register_fn(ctx). A plugin that directly reads a missing variable can raise into the existing generic failure handler (hermes_cli/plugins.py:1824-1829) before the declared variable is reported. Move the normalization/warning before registration and cover a raisingregister()test. - This does not fix the linked Hindsight path: general discovery skips
plugins/memory/(hermes_cli/plugins.py:1329-1340), Hindsight declaresrequires_env: [](plugins/memory/hindsight/plugin.yaml:6), andplugins.memoryowns its loader (plugins/memory/__init__.py:157-215). Please narrow the claim or add the diagnostic at that loader. - The provider-only test's
register()is a no-op, not a provider registration. Provider registrations are not represented inLoadedPlugin(hermes_cli/plugins.py:323-326; image providers register via:648-671).
Suggested changes
- Warn before calling
register_fn(ctx)and add a throwing-registration regression test. - Add a real provider-registration test, and separately decide whether the zero-registration diagnostic belongs in the memory-provider path.
Automated hermes-sweeper review.
|
|
||
| # Warn when all required env vars are present but the plugin | ||
| # registered nothing on any surface. Normalize requires_env | ||
| # entries (str or dict) before checking the process environment, |
There was a problem hiding this comment.
This validation runs after register_fn(ctx). Move missing-env normalization and its warning before registration: a plugin that accesses a missing required variable directly will raise into the generic load-error path before this branch can name the declared variable.
| (plugin_dir / "__init__.py").write_text( | ||
| "def register(ctx):\n pass # registers a provider internally\n" | ||
| ) | ||
|
|
There was a problem hiding this comment.
This is a no-op registration rather than a provider registration, so it cannot verify the stated provider-only case. Exercise a real supported provider registration path, or name this test as a no-op plugin test.
|
Ported forward in #68703: warning now fires before register_fn(ctx) so a plugin that crashes reading the missing var still reports the specific diagnostic, and the provider-only test now uses a real ImageGenProvider registration instead of a no-op. Narrowed the Hindsight claim -- that path uses a separate loader this PR does not touch. Closing in favor of #68703. |
Problem
_load_plugin() emitted only a DEBUG registration count, so a plugin whose required env vars were absent loaded silently (issue #2768).
Fix
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. The empty-registration note is demoted to DEBUG to avoid false positives for provider-only plugins (e.g. image_gen/fal) whose registrations are not tracked in LoadedPlugin.
Addresses all three points from the maintainer review of #2768: requires_env normalization for both manifest forms, correct registration surface accounting (no false WARNING for providers), and tests ported to tests/hermes_cli/test_plugins.py.
Verification
4 new tests: str-form missing env, dict-form missing env, present env no warning, provider-only no false-positive. 4/4 pass alongside 100 existing plugin tests.
Fixes #2768