mirror of
https://github.com/NousResearch/hermes-agent.git
synced 2026-11-06 05:17:15 +00:00
fix(memory): keep refused saves a 400 across the plugin host; hooks survive SystemExit
- plugin_host_wire: an error crossing the host now carries the nearest builtin base class, so a ValueError subclass (json.JSONDecodeError) raised by a hosted provider's write_config_fields is still a ValueError in core and the dashboard returns 400, not 500. - notify_memory_provider catches SystemExit as well as Exception: a provider hook that exits no longer skips rename's alias, active-profile and identity updates. - Docs and docstring: a key missing from the target profile's .env reads as unset; the hooks never fall back to the caller's environment. - Memory panel GET maps a provider read failure like PUT does (ValueError -> 400). - One helper for submitted secrets; the Honcho placeholder rule moves into its storage branch so the field loop no longer branches on storage kind. - Tests: wire round-trip of error classes, SystemExit hook, missing-secret fail-closed, and the failing-hook test now asserts both hooks ran.
This commit is contained in:
1 parent
e090092285
commit
ddf69149ed
8 files changed
+129
-27
No files matched your search
@@ -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")
|
||||
@@ -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)
|
||||
|
||||
|
||||
@@ -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:
|
||||
|
||||
@@ -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")
|
||||
|
||||
@@ -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))
|
||||
@@ -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"]
|
||||
@@ -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
|
||||
|
||||
@@ -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.
|
||||
...
|
||||
```
|
||||
|
||||
|
||||
Reference in new issue
Block a user