diff --git a/agent/memory_provider.py b/agent/memory_provider.py index 2aba900920..ec5d2c391e 100644 --- a/agent/memory_provider.py +++ b/agent/memory_provider.py @@ -248,5 +248,6 @@ class MemoryProvider(ABC): def write_config_fields(self, values: dict[str, Any], hermes_home: str) -> None: """Persist the submitted fields (only those present): natives already coerced, ``None`` clears a key, non-blank secrets included (core saves each to ``.env`` under its ``env_key`` once this - returns). Raise ``ValueError`` to refuse the save (the dashboard shows it as a 400).""" + returns). Raise ``ValueError`` (a subclass such as ``json.JSONDecodeError`` counts, also under + ``plugins.isolation: host``) to refuse the save (the dashboard shows it as a 400).""" raise NotImplementedError(f"memory provider {self.name} declares storage=provider but cannot write it") diff --git a/hermes_cli/plugin_host_wire.py b/hermes_cli/plugin_host_wire.py index f59b0631cf..8efc10fc4b 100644 --- a/hermes_cli/plugin_host_wire.py +++ b/hermes_cli/plugin_host_wire.py @@ -63,11 +63,21 @@ _REMOTE_EXCEPTIONS = { _REMOTE_EXCEPTIONS["PluginHostUnsupported"] = PluginHostUnsupported +def error_payload(exc: BaseException) -> dict[str, Any]: + """The wire form of ``exc``: its class name, plus the nearest builtin ``raise_remote`` restores + (so a ``json.JSONDecodeError`` is still caught by ``except ValueError`` on the other side).""" + payload: dict[str, Any] = {"type": type(exc).__name__, "message": str(exc)} + base = next((c.__name__ for c in type(exc).__mro__ if c.__name__ in _REMOTE_EXCEPTIONS), None) + if base and base != payload["type"]: + payload["base"] = base + return payload + + def raise_remote(error: dict[str, Any]) -> None: - """Re-raise a remote error, as the builtin type when it is one plugins commonly catch.""" + """Re-raise a remote error, as the builtin type (or nearest builtin base) plugins commonly catch.""" type_name = str(error.get("type") or "") message = str(error.get("message") or "") - cls = _REMOTE_EXCEPTIONS.get(type_name) + cls = _REMOTE_EXCEPTIONS.get(type_name) or _REMOTE_EXCEPTIONS.get(str(error.get("base") or "")) if cls is not None: raise cls(message) raise PluginHostError(type_name, message) @@ -374,7 +384,7 @@ class Channel: if not isinstance(exc, Exception): logger.warning("%s: request %s raised %s", self._name, message.get("method"), type(exc).__name__) - return {"id": request_id, "error": {"type": type(exc).__name__, "message": str(exc)}} + return {"id": request_id, "error": error_payload(exc)} finally: _SERVING.reset(token) diff --git a/hermes_cli/profile_memory_config.py b/hermes_cli/profile_memory_config.py index c80e5187bd..6692e56be7 100644 --- a/hermes_cli/profile_memory_config.py +++ b/hermes_cli/profile_memory_config.py @@ -81,13 +81,12 @@ def _load_config_dict(profile_dir: Path) -> Optional[dict]: def notify_memory_provider(profile_dir: Path, call: Callable[[MemoryProvider], None]) -> None: """Run ``call`` on the memory provider *profile_dir*'s config selects, with that profile's home AND - its own secrets bound (a profile lifecycle hook): ``get_secret`` reads the new profile's ``.env`` - first (outside multiplexing, a key it lacks falls back to the process env, as at runtime). No + its own secrets bound (a profile lifecycle hook): ``get_secret`` reads the target profile's + ``.env`` and never falls back to the caller's environment for a key it lacks. No provider selected or installed: nothing runs. Best-effort: a failing provider, or an unreadable config, is logged and never fails the create/rename that called it (the directory is already published).""" from agent.secret_scope import build_profile_secret_scope, reset_secret_scope, set_secret_scope - from hermes_cli.config_backend import ConfigBackendUnavailable from hermes_constants import reset_hermes_home_override, set_hermes_home_override from plugins.memory import load_memory_provider @@ -104,7 +103,7 @@ def notify_memory_provider(profile_dir: Path, call: Callable[[MemoryProvider], N provider = load_memory_provider(name, register_skills=False) if provider is not None: call(provider) - except (Exception, ConfigBackendUnavailable): # the backend error is a SystemExit on purpose + except (Exception, SystemExit): # ConfigBackendUnavailable is a SystemExit; a plugin may sys.exit logger.warning("memory provider %r: profile hook failed for %s", name, profile_dir, exc_info=True) finally: if secret_token is not None: diff --git a/hermes_cli/web_routers/memory_providers.py b/hermes_cli/web_routers/memory_providers.py index 436bb0fb66..0694daf604 100644 --- a/hermes_cli/web_routers/memory_providers.py +++ b/hermes_cli/web_routers/memory_providers.py @@ -139,17 +139,19 @@ def _provider_instance(provider: ProviderConfigSchema): return instance +def _submitted_secrets(provider: ProviderConfigSchema, values: dict[str, str]) -> list: + """The ``(field, submitted)`` pairs of each non-blank secret submission.""" + pairs = ((field, (values.get(field.key) or "").strip()) for field in provider.fields if field.is_secret) + return [(field, submitted) for field, submitted in pairs if submitted] + + def _save_submitted_secrets(provider: ProviderConfigSchema, values: dict[str, str]) -> list: """Persist each non-blank secret submission to the env store (when the field has an ``env_key``); return the ``(field, submitted)`` pairs for backend-specific handling.""" - saved = [] - for field in provider.fields: - submitted = (values.get(field.key) or "").strip() if field.is_secret else "" - if not submitted: - continue + saved = _submitted_secrets(provider, values) + for field, submitted in saved: if field.env_key: save_env_value(field.env_key, submitted) - saved.append((field, submitted)) return saved @@ -190,8 +192,7 @@ def _submitted_natives(provider: ProviderConfigSchema, values: dict[str, str]) - def _write_provider_own(provider: ProviderConfigSchema, values: dict[str, str]) -> None: """Hand the submitted fields to the provider's ``write_config_fields`` (natives, ``None`` = clear, non-blank secrets included); secrets reach ``.env`` only once the provider accepted the write.""" - secrets = {field.key: (values.get(field.key) or "").strip() for field in provider.fields if field.is_secret} - fields = {k: v for k, v in secrets.items() if v} + fields: dict[str, Any] = {field.key: submitted for field, submitted in _submitted_secrets(provider, values)} fields.update({k: None if v is _UNSET else v for k, v in _submitted_natives(provider, values).items()}) _provider_instance(provider).write_config_fields(fields, str(get_hermes_home())) _save_submitted_secrets(provider, values) @@ -279,7 +280,7 @@ def _declared_provider_payload(provider: ProviderConfigSchema) -> dict[str, Any] placeholders: dict[str, Any] = {} if own: stored = _provider_instance(provider).read_config_fields(str(get_hermes_home())) or {} - host, data = "", dict(stored.get("values") or {}) + data = dict(stored.get("values") or {}) placeholders = dict(stored.get("placeholders") or {}) def sources_for(field: ProviderField) -> tuple: @@ -287,13 +288,15 @@ def _declared_provider_payload(provider: ProviderConfigSchema) -> dict[str, Any] elif is_honcho: resolve_active_host, resolve_config_path, host_block_of = _honcho_resolvers(provider.name) host = resolve_active_host() + # Blank fields surface the resolved host Honcho will actually use. + placeholders = {"workspace": host, "aiPeer": host} raw = _read_json_dict(resolve_config_path(), "Honcho config") host_block = host_block_of(raw, host) def sources_for(field: ProviderField) -> tuple: return (host_block, raw) if field.scope == "host" else (raw,) else: - host, data = "", _read_flat_json(provider) + data = _read_flat_json(provider) def sources_for(field: ProviderField) -> tuple: return (data,) @@ -311,9 +314,6 @@ def _declared_provider_payload(provider: ProviderConfigSchema) -> dict[str, Any] fields.append(entry) continue native = _read_field(field, sources, env) - if is_honcho and not field.placeholder and field.key in {"workspace", "aiPeer"}: - # Blank fields surface the resolved host Honcho will actually use. - entry["placeholder"] = host value = _serialize_field_value(field, native) if field.kind == "select" and value not in field.allowed_values(): value = field.default @@ -561,7 +561,8 @@ async def get_memory_provider_config(name: str, surface: Optional[str] = None, p return {"name": name, "label": name, "fields": [], "setup": _memory_provider_setup_info(name)} return _memory_provider_payload(name, provider) - return await scoped_to_thread(profile, _run) + with _value_errors_as_http("GET /api/memory/providers/%s/config failed", name): + return await scoped_to_thread(profile, _run) @router.post("/api/memory/providers/{name}/setup") diff --git a/tests/hermes_cli/test_plugin_host_wire_errors.py b/tests/hermes_cli/test_plugin_host_wire_errors.py new file mode 100644 index 0000000000..de15bee372 --- /dev/null +++ b/tests/hermes_cli/test_plugin_host_wire_errors.py @@ -0,0 +1,39 @@ +"""Errors crossing the plugin-host wire keep the builtin class callers catch.""" + +import json + +import pytest + +from hermes_cli.plugin_host_wire import PluginHostError, error_payload, raise_remote + + +def _roundtrip(exc: BaseException): + raise_remote(json.loads(json.dumps(error_payload(exc)))) + + +def test_a_value_error_subclass_is_still_a_value_error_on_the_other_side(): + try: + json.loads("{not json") + except json.JSONDecodeError as exc: + original = exc + with pytest.raises(ValueError, match="Expecting property name"): + _roundtrip(original) + + +def test_an_exact_builtin_keeps_its_class(): + with pytest.raises(KeyError): + _roundtrip(KeyError("k")) + + +def test_an_unmapped_error_is_a_plugin_host_error(): + class Custom(Exception): + pass + + with pytest.raises(PluginHostError) as info: + _roundtrip(Custom("x")) + assert info.value.type_name == "Custom" + + +def test_system_exit_is_not_mapped_to_a_builtin(): + with pytest.raises(PluginHostError): + _roundtrip(SystemExit(3)) diff --git a/tests/hermes_cli/test_profiles_memory_hooks.py b/tests/hermes_cli/test_profiles_memory_hooks.py index 9bcf51056f..c5559340d8 100644 --- a/tests/hermes_cli/test_profiles_memory_hooks.py +++ b/tests/hermes_cli/test_profiles_memory_hooks.py @@ -111,17 +111,68 @@ def test_an_unreadable_config_backend_never_fails_create(profile_env, monkeypatc assert "profile hook failed" in caplog.text +def test_a_missing_profile_secret_never_falls_back_to_the_callers_env(profile_env, monkeypatch): + import plugins.memory as memory_pkg + from agent.secret_scope import get_secret + from hermes_cli.profiles import create_profile + + seen = [] + + class Reader(_Stub): + name = "reader" + + def on_profile_created(self, profile_name, hermes_home): + seen.append(get_secret("PROBE_PROFILE_KEY", "unset")) + + monkeypatch.setattr(memory_pkg, "load_memory_provider", lambda name, **kw: Reader()) + monkeypatch.setenv("PROBE_PROFILE_KEY", "caller-value") + _select(profile_env, "reader") + create_profile("work", clone_config=True, no_alias=True) # the cloned .env has no such key + assert seen == ["unset"] + + +def test_a_provider_hook_that_exits_never_fails_create_or_rename(profile_env, monkeypatch): + import plugins.memory as memory_pkg + from hermes_cli.profiles import create_profile, rename_profile + from hermes_constants import get_hermes_home + + class Exits(_Stub): + name = "exits" + + def on_profile_created(self, profile_name, hermes_home): + calls.append("created") + raise SystemExit(3) + + def on_profile_renamed(self, old_name, new_name, hermes_home): + calls.append("renamed") + raise SystemExit(3) + + home = get_hermes_home() + calls = [] + monkeypatch.setattr(memory_pkg, "load_memory_provider", lambda name, **kw: Exits()) + _select(profile_env, "exits") + assert create_profile("work", clone_config=True, no_alias=True).is_dir() + with patch("hermes_cli.profiles.check_alias_collision", return_value="skip"): + assert rename_profile("work", "play").is_dir() + assert calls == ["created", "renamed"] + assert get_hermes_home() == home # the profile home override was reset + + def test_a_failing_provider_hook_never_fails_create_or_rename(profile_env, monkeypatch): import plugins.memory as memory_pkg from hermes_cli.profiles import create_profile, rename_profile + calls = [] + class Broken(_Stub): name = "broken" def on_profile_created(self, profile_name, hermes_home): + calls.append("created") raise RuntimeError("boom") def on_profile_renamed(self, old_name, new_name, hermes_home): + calls.append("renamed") raise RuntimeError("boom") monkeypatch.setattr(memory_pkg, "load_memory_provider", lambda name, **kw: Broken()) @@ -129,4 +180,4 @@ def test_a_failing_provider_hook_never_fails_create_or_rename(profile_env, monke create_profile("work", clone_config=True, no_alias=True) with patch("hermes_cli.profiles.check_alias_collision", return_value="skip"): assert rename_profile("work", "play").is_dir() - + assert calls == ["created", "renamed"] diff --git a/tests/hermes_cli/test_web_memory_provider_storage.py b/tests/hermes_cli/test_web_memory_provider_storage.py index 4171cbc260..7e0eebdead 100644 --- a/tests/hermes_cli/test_web_memory_provider_storage.py +++ b/tests/hermes_cli/test_web_memory_provider_storage.py @@ -39,7 +39,7 @@ class OwnProv(MemoryProvider): return {"values": values, "placeholders": {"workspace": "hermes", "aiPeer": "hermes"}} def write_config_fields(self, values, hermes_home): - path, cfg = _read(hermes_home) # a file that does not parse raises ValueError: no write + path, cfg = _read(hermes_home) # a file that does not parse raises JSONDecodeError: no write block = cfg.setdefault("hosts", {}).setdefault("hermes", {}) for key, value in values.items(): target = cfg if key in _ROOT_KEYS else block diff --git a/website/docs/developer-guide/memory-provider-plugin.md b/website/docs/developer-guide/memory-provider-plugin.md index a16aa4448c..79d5eaec15 100644 --- a/website/docs/developer-guide/memory-provider-plugin.md +++ b/website/docs/developer-guide/memory-provider-plugin.md @@ -182,7 +182,8 @@ workspace; absent or empty cwd remains unpinned. | `on_profile_renamed(old_name, new_name, hermes_home)` | That profile was renamed (`hermes_home` is the new directory) | Re-key state stored under the old name | The two profile hooks run on an uninitialized instance with that profile's home and its own `.env` -secrets bound (a key that `.env` lacks falls back to the process environment, as at runtime). An exception is logged and never fails the create or rename. They run inside the +secrets bound. A key that profile's `.env` lacks reads as unset: the caller's environment is never +used. An exception (`SystemExit` included) is logged and never fails the create or rename. They run inside the create/rename request on every surface, so do network work on a background thread. For native `replace` and `remove`, `metadata["previous_content"]` contains the full @@ -359,8 +360,8 @@ def read_config_fields(self, hermes_home: str) -> dict: def write_config_fields(self, values: dict, hermes_home: str) -> None: # Only submitted keys; None clears one. Non-blank secrets arrive here too; core saves them to - # .env under env_key only after this returns. Raise a plain ValueError to refuse the save - # (shown as a 400), e.g. when your file does not parse. + # .env under env_key only after this returns. Raise ValueError (a subclass such as + # json.JSONDecodeError counts) to refuse the save (shown as a 400), e.g. when your file does not parse. ... ```