diff --git a/AGENTS.md b/AGENTS.md index 8230d860aa..2146b95dff 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -797,6 +797,31 @@ as a side effect of importing `model_tools.py`. Code paths that read plugin state without importing `model_tools.py` first must call `discover_plugins()` explicitly (it's idempotent). +#### Native plugin compatibility policy + +The canonical contract and deprecation policy live in +`website/docs/developer-guide/plugins/index.md#native-plugin-compatibility-contract`. +Compatibility is enforced as a behavior contract, not through a monolithic +`PLUGIN_API_VERSION`, a manifest-wide native `api:` match, or version literals +on unrelated payloads. Keep documented plugin surfaces additive: + +- add hook payload data as keyword fields; signature-inspect callbacks so old + narrow signatures receive only fields they declare, while `**kwargs` + callbacks receive the complete payload; +- do not remove or rename `PluginContext` methods; make new parameters optional + with defaults and keyword-only where possible; +- ignore unknown native manifest fields; +- give new provider methods default implementations, and signature-inspect + optional callback kwargs rather than forwarding them unconditionally; +- use a local schema version only for a capability with a wire or persisted + contract, and preserve old state/config/session replay or ship a migration. + +Deprecations require a once-per-process warning, a documented replacement and +migration note, and at least two subsequent minor releases before removal. +Compatibility tests must load frozen plugins through the real discovery path +and assert outcomes. Do not replace these with exact registry/catalog counts, +source-reading tests, or assertions that a global version literal changed. + ### Memory-provider plugins (`plugins/memory//`) Separate discovery system for pluggable memory backends. Current built-in diff --git a/hermes_cli/plugins.py b/hermes_cli/plugins.py index 54ffde10ee..b96820f742 100644 --- a/hermes_cli/plugins.py +++ b/hermes_cli/plugins.py @@ -2162,11 +2162,42 @@ class PluginManager: # Hook invocation # ----------------------------------------------------------------------- + @staticmethod + def _invoke_hook_callback(callback: Callable, payload: Dict[str, Any]) -> Any: + """Invoke a hook while withholding additive fields from old callbacks.""" + try: + parameters = inspect.signature(callback).parameters + except (TypeError, ValueError): + # Some extension/builtin callables do not expose a signature. Keep + # the historical behavior for those callables rather than guessing. + return callback(**payload) + + if any( + parameter.kind == inspect.Parameter.VAR_KEYWORD + for parameter in parameters.values() + ): + return callback(**payload) + + accepted_payload = { + name: value + for name, value in payload.items() + if name in parameters + and parameters[name].kind + in { + inspect.Parameter.POSITIONAL_OR_KEYWORD, + inspect.Parameter.KEYWORD_ONLY, + } + } + return callback(**accepted_payload) + def invoke_hook(self, hook_name: str, **kwargs: Any) -> List[Any]: """Call all registered callbacks for *hook_name*. - Each callback is wrapped in its own try/except so a misbehaving - plugin cannot break the core agent loop. + Hook payloads evolve additively. Callbacks that accept ``**kwargs`` + receive the complete payload; older callbacks with a narrow signature + receive only the keyword arguments they declare. Each callback is + wrapped in its own try/except so a misbehaving plugin cannot break the + core agent loop. Returns a list of non-``None`` return values from callbacks. @@ -2187,7 +2218,7 @@ class PluginManager: results: List[Any] = [] for cb in callbacks: try: - ret = cb(**kwargs) + ret = self._invoke_hook_callback(cb, kwargs) if ret is not None: results.append(ret) except Exception as exc: diff --git a/tests/agent/test_memory_provider.py b/tests/agent/test_memory_provider.py index d11cfc026d..a7e8840ff5 100644 --- a/tests/agent/test_memory_provider.py +++ b/tests/agent/test_memory_provider.py @@ -216,6 +216,26 @@ class TestMemoryManager: # p1 failed but p2 still synced assert p2.synced_turns == [("user", "assistant")] + def test_sync_all_keeps_legacy_provider_working_when_messages_are_available(self): + """New optional turn context is not sent to an old provider signature.""" + mgr = MemoryManager() + legacy_provider = FakeMemoryProvider("legacy") + mgr.add_provider(legacy_provider) + completed_messages = [ + {"role": "user", "content": "user"}, + {"role": "assistant", "content": "assistant"}, + ] + + mgr.sync_all( + "user", + "assistant", + session_id="legacy-session", + messages=completed_messages, + ) + mgr.flush_pending(timeout=5) + + assert legacy_provider.synced_turns == [("user", "assistant")] + # -- Tool routing ------------------------------------------------------- diff --git a/tests/hermes_cli/fixtures/plugin_compat_legacy/__init__.py b/tests/hermes_cli/fixtures/plugin_compat_legacy/__init__.py new file mode 100644 index 0000000000..42b504ad53 --- /dev/null +++ b/tests/hermes_cli/fixtures/plugin_compat_legacy/__init__.py @@ -0,0 +1,16 @@ +"""Frozen legacy plugin used by the behavior compatibility suite. + +Keep this callback in its old, narrow form: it intentionally predates additive +hook payload fields and does not accept ``**kwargs``. +""" + +received_sessions = [] + + +def _on_session_start(*, session_id): + received_sessions.append(session_id) + return {"legacy_session_id": session_id} + + +def register(ctx): + ctx.register_hook("on_session_start", _on_session_start) diff --git a/tests/hermes_cli/fixtures/plugin_compat_legacy/plugin.yaml b/tests/hermes_cli/fixtures/plugin_compat_legacy/plugin.yaml new file mode 100644 index 0000000000..01d7fb1464 --- /dev/null +++ b/tests/hermes_cli/fixtures/plugin_compat_legacy/plugin.yaml @@ -0,0 +1,8 @@ +name: legacy-contract-fixture +version: "0.1.0" +description: Frozen plugin fixture for native behavior compatibility +provides_hooks: + - on_session_start +future_manifest_field: + schema: 2 + additive: true diff --git a/tests/hermes_cli/test_plugin_api_compat.py b/tests/hermes_cli/test_plugin_api_compat.py new file mode 100644 index 0000000000..d0024dce2a --- /dev/null +++ b/tests/hermes_cli/test_plugin_api_compat.py @@ -0,0 +1,52 @@ +"""Behavior-contract compatibility tests for native Hermes plugins.""" + +from pathlib import Path +import shutil + +import yaml + +from hermes_cli.plugins import PluginManager + + +LEGACY_PLUGIN = Path(__file__).parent / "fixtures" / "plugin_compat_legacy" + + +def test_legacy_plugin_loads_and_ignores_additive_hook_and_manifest_fields( + tmp_path, monkeypatch +): + """A frozen plugin keeps working as manifests and hook payloads grow.""" + hermes_home = tmp_path / "hermes-home" + plugins_dir = hermes_home / "plugins" + plugins_dir.mkdir(parents=True) + shutil.copytree(LEGACY_PLUGIN, plugins_dir / "legacy-contract-fixture") + (hermes_home / "config.yaml").write_text( + yaml.safe_dump( + {"plugins": {"enabled": ["legacy-contract-fixture"]}} + ), + encoding="utf-8", + ) + empty_bundled = tmp_path / "bundled-plugins" + empty_bundled.mkdir() + monkeypatch.setenv("HOME", str(tmp_path / "os-home")) + monkeypatch.setenv("HERMES_HOME", str(hermes_home)) + monkeypatch.setenv("HERMES_BUNDLED_PLUGINS", str(empty_bundled)) + + manager = PluginManager() + manager.discover_and_load() + + loaded = manager._plugins["legacy-contract-fixture"] + assert loaded.enabled is True + assert loaded.error is None + assert loaded.module is not None + assert loaded.manifest.version == "0.1.0" + assert "on_session_start" in loaded.hooks_registered + + results = manager.invoke_hook( + "on_session_start", + session_id="legacy-session", + resumed=True, + future_additive_field={"nested": "value"}, + ) + + assert results == [{"legacy_session_id": "legacy-session"}] + assert loaded.module.received_sessions == ["legacy-session"] diff --git a/website/docs/developer-guide/plugins/index.md b/website/docs/developer-guide/plugins/index.md index c16e079354..1b645a29f3 100644 --- a/website/docs/developer-guide/plugins/index.md +++ b/website/docs/developer-guide/plugins/index.md @@ -103,6 +103,65 @@ records it as Published. Hermes keys behavior on the canonical v1.0.0 schema identifiers and normative text, not either mutable status label. This is an explicit supported subset, not a claim of full Agent Plugins conformance. +## Native plugin compatibility contract + +Native `plugin.yaml` plus `register(ctx)` plugins are protected by behavior, +not by one global plugin API number. Hermes does not expose a +`PLUGIN_API_VERSION`, require a manifest-wide `api:` match, or attach an API +version to unrelated values. A plugin that uses a documented behavior should +continue to work after a normal Hermes upgrade. + +The compatibility rules are: + +- **Evolve additively.** Documented `PluginContext` methods are not removed or + renamed. New parameters are optional, have defaults, and should be + keyword-only. Existing return fields are not removed or silently retyped. +- **Hook payloads are keyword payloads.** New hook data is added as keyword + fields, never by changing the meaning or position of an existing field. + Hermes inspects callback signatures: a legacy callback receives the fields it + declares, while a callback with `**kwargs` receives the complete current + payload. New plugins should accept `**kwargs` so they can opt into additive + data without another signature change. +- **Manifests are open to additions.** Unknown `plugin.yaml` fields are ignored. + Older Hermes releases can therefore load a plugin whose manifest contains + metadata introduced by a newer release, provided the plugin code itself uses + supported runtime behavior. +- **Provider interfaces grow through defaults.** New provider methods have a + default implementation. New callback context is optional and forwarded only + when signature inspection shows that a provider accepts it. Adding an + abstract method or an unconditionally forwarded argument requires a + migration window rather than a flag-day signature change. +- **Version the contract that crosses a boundary.** A capability may carry its + own schema version when it defines a wire payload or persisted format (for + example, observer payloads or secret-source state). Keep fields additive + within that local schema. Persisted plugin state and config must remain + readable, or ship an explicit migration; resumed sessions written by the old + format must still replay. Do not add version literals to unrelated callback + or context values. + +### Deprecation policy + +A documented native plugin behavior may be deprecated only with all of the +following: + +1. a replacement and migration instructions in the plugin guide and release + notes; +2. a warning emitted at most once per process, naming the replacement and the + earliest removal release; +3. support for the old behavior through at least two subsequent minor + releases; and +4. behavior-based compatibility coverage for both the legacy path and the + replacement throughout that window. + +Removal after the window must include any migration needed for persisted data +or resumable sessions. In practice, additive aliases and adapters are preferred +to removal. + +Hermes enforces this contract with frozen external-plugin fixtures discovered +from an isolated `HERMES_HOME`. Those tests load and invoke the plugin through +`PluginManager`; they assert real registration and callback outcomes rather +than internal symbol lists or source-code shape. + ## What you're building A **calculator** plugin with two tools: