From 995ad7f658a08bcfc16238dfb1b477c62d197a63 Mon Sep 17 00:00:00 2001 From: PowerCreek Date: Sat, 23 May 2026 03:12:58 +0000 Subject: [PATCH] memory: holographic health_check + collapse doctor branches (#42 steps 2h + 3) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Closes #42. Final two steps of the RFC migration. Step 2h — holographic health_check: Holographic is local-only (SQLite); no remote service. But a read-only HERMES_HOME (RO mount, wrong perms) would still break it at runtime. Override verifies db_path's parent directory is writable. (True, "") — parent mkdir + W_OK pass. (False, "unreachable: ") — mkdir failed or W_OK denied. (False, "config_error: ") — get_hermes_home raised. Step 3 — doctor branch collapse: Every shipped provider now implements health_check() with the RFC #42 reason-prefix taxonomy. The provider-specific Honcho + Mem0 elif blocks (~120 lines combined) are gone. The unified dispatch is one ~80-line block in doctor.py keyed on the prefix: healthy → check_ok(" reachable") no_api_key / no_credentials → _fail_and_issue (setup hint) no_url / no_endpoint → _fail_and_issue (URL hint) no_config → check_warn (run setup) disabled → check_info sdk_missing → _fail_and_issue (plugin docs) auth: → _fail_and_issue (rotate key) not_found: → _fail_and_issue (verify base URL) http: → check_warn (unexpected status) unreachable: → check_warn (transient hint) config_error: → check_warn (config raised) health_check_raised: → check_warn (provider bug; RFC #42 says health_check MUST NOT raise, so this is a contract violation worth flagging) other → check_warn (unknown verbatim) Doctor's Memory Provider section is now ~80 lines instead of ~200, and adding the 9th provider requires zero doctor changes. Migration table (8 providers total): mem0 — PR #44 GET /v1/memory profile honcho — PR #45 get_honcho_client handshake byterover — PR #46 brv status (CLI + login) supermemory — PR #48 client.profile probe openviking — PR #49 /health endpoint probe retaindb — PR #50 /v1/memory/profile GET hindsight — PR #51 mode-dependent (local import / cloud /version) holographic — this PR db_path parent writability Tests: - 6 new tests for holographic health_check - Updated 3 doctor tests to assert the new unified dispatch (auth → _fail_and_issue; unreachable → check_warn; healthy → " reachable") instead of the now-removed elif-block output strings. - 228 health_check + doctor tests pass total. The #42 RFC is now fully implemented across all shipped memory providers. Closing the issue with this PR. --- hermes_cli/doctor.py | 219 ++++++++---------- plugins/memory/holographic/__init__.py | 42 ++++ tests/hermes_cli/test_doctor.py | 62 ++++- .../memory/test_holographic_health_check.py | 97 ++++++++ 4 files changed, 280 insertions(+), 140 deletions(-) create mode 100644 tests/plugins/memory/test_holographic_health_check.py diff --git a/hermes_cli/doctor.py b/hermes_cli/doctor.py index 02e3f73ecb56..a14ed90db485 100644 --- a/hermes_cli/doctor.py +++ b/hermes_cli/doctor.py @@ -2235,143 +2235,106 @@ def _gh_authenticated() -> bool: if not _active_memory_provider: check_ok("Built-in memory active", "(no external provider configured — this is fine)") - elif _active_memory_provider == "honcho": + else: + # Unified memory-provider probe (#42). Every shipped provider + # now implements `health_check() -> (bool, reason)` with the + # RFC #42 reason-prefix taxonomy. Doctor dispatches on the + # prefix to choose check_ok / _fail_and_issue / check_warn. try: - from plugins.memory.honcho.client import HonchoClientConfig, resolve_config_path - hcfg = HonchoClientConfig.from_global_config() - _honcho_cfg_path = resolve_config_path() - - if not _honcho_cfg_path.exists(): - check_warn("Honcho config not found", "run: hermes memory setup") - elif not hcfg.enabled: - check_info(f"Honcho disabled (set enabled: true in {_honcho_cfg_path} to activate)") - elif not (hcfg.api_key or hcfg.base_url): + from plugins.memory import load_memory_provider + _provider = load_memory_provider(_active_memory_provider) + except Exception as _e: + _provider = None + check_warn(f"{_active_memory_provider} plugin not loadable", + str(_e)) + _provider = None + + if _provider is None and _active_memory_provider: + # load_memory_provider returned None — plugin not on disk. + # Skip the dispatch below (the except-branch above already + # surfaced a warn if the load itself raised). + pass + elif _provider is None: + check_warn(f"{_active_memory_provider} plugin not found", + "run: hermes memory setup") + else: + try: + _healthy, _reason = _provider.health_check() + except Exception as _e: # noqa: BLE001 + # RFC #42 contract says health_check MUST NOT raise. + # A propagating exception is a provider bug — surface + # it as a warn and keep going. + _healthy, _reason = False, f"health_check_raised: {_e}" + + _pname = _active_memory_provider + _prefix, _, _detail = _reason.partition(":") + _detail = _detail.strip() + if _healthy: + check_ok(f"{_pname} reachable") + elif _reason == "no_api_key" or _reason == "no_credentials": _fail_and_issue( - "Honcho API key or base URL not set", + f"{_pname} not configured", "run: hermes memory setup", - "No Honcho API key — run 'hermes memory setup'", + f"{_pname} is set as memory provider but credentials are missing", issues, ) - else: - from plugins.memory.honcho.client import get_honcho_client, reset_honcho_client - reset_honcho_client() - try: - get_honcho_client(hcfg) - check_ok( - "Honcho connected", - f"workspace={hcfg.workspace_id} mode={hcfg.recall_mode} freq={hcfg.write_frequency}", - ) - except Exception as _e: - _fail_and_issue("Honcho connection failed", str(_e), f"Honcho unreachable: {_e}", issues) - except ImportError: - _fail_and_issue( - "honcho-ai not installed", - "pip install honcho-ai", - "Honcho is set as memory provider but honcho-ai is not installed", - issues, - ) - except Exception as _e: - check_warn("Honcho check failed", str(_e)) - elif _active_memory_provider == "mem0": - try: - from plugins.memory.mem0 import _load_config as _load_mem0_config - mem0_cfg = _load_mem0_config() - mem0_key = mem0_cfg.get("api_key", "") - mem0_user = mem0_cfg.get("user_id", "hermes-user") - mem0_agent = mem0_cfg.get("agent_id", "hermes") - if not mem0_key: + elif _reason == "no_url" or _reason == "no_endpoint": _fail_and_issue( - "Mem0 API key not set", - "(set MEM0_API_KEY in .env or run hermes memory setup)", - "Mem0 is set as memory provider but API key is missing", + f"{_pname} URL not set", + "set the provider's *_BASE_URL / *_API_URL / " + "*_ENDPOINT env var, or run: hermes memory setup", + f"{_pname} URL not configured", issues, ) + elif _reason == "no_config": + check_warn(f"{_pname} config not found", + "run: hermes memory setup") + elif _reason == "disabled": + check_info(f"{_pname} disabled in config") + elif _reason == "sdk_missing" or _prefix == "sdk_missing": + _fail_and_issue( + f"{_pname} SDK not installed", + "see the provider's plugin docs for the correct " + "`pip install` package", + f"{_pname} SDK missing — see plugin docs", + issues, + ) + elif _prefix == "auth": + _fail_and_issue( + f"{_pname} auth rejected", + _detail[:200], + f"{_pname} authentication rejected — verify the " + "configured API key / token", + issues, + ) + elif _prefix == "not_found": + _fail_and_issue( + f"{_pname} endpoint not found", + _detail[:200], + f"{_pname} returned 404 — verify the configured base URL", + issues, + ) + elif _prefix == "http": + check_warn(f"{_pname} unexpected HTTP status", + _detail[:200]) + elif _prefix == "unreachable": + check_warn(f"{_pname} unreachable", + f"{_detail[:200]} " + "(network down, server restarting, or " + "transient — credentials may still be valid)") + elif _prefix == "config_error": + check_warn(f"{_pname} config error", + _detail[:200]) + elif _prefix == "health_check_raised": + check_warn(f"{_pname} health_check raised", + f"{_detail[:200]} (RFC #42 contract: " + "health_check must not raise; " + "treat as provider bug)") else: - # Probe with a minimal API call so a wrong / expired key - # surfaces here instead of failing at first-request time - # in production (see #34). Honcho already does this via - # get_honcho_client(); Mem0 didn't. - try: - from mem0 import MemoryClient - _mem0_client = MemoryClient(api_key=mem0_key) - _mem0_client.get_all(user_id=mem0_user, limit=1) - except ImportError: - _fail_and_issue( - "mem0ai not installed", - "pip install mem0ai", - "Mem0 is set as memory provider but mem0ai SDK is not installed", - issues, - ) - except Exception as _mp_exc: # noqa: BLE001 - msg = str(_mp_exc) - msg_lc = msg.lower() - is_auth = ( - "401" in msg - or "403" in msg - or "unauthorized" in msg_lc - or "forbidden" in msg_lc - or "invalid api key" in msg_lc - or "authentication" in msg_lc - ) - if is_auth: - _fail_and_issue( - "Mem0 auth rejected", - msg[:200], - "Mem0 API key rejected — verify MEM0_API_KEY " - "at https://app.mem0.ai", - issues, - ) - else: - check_warn( - "Mem0 probe failed", - f"{msg[:200]} " - "(network down, SDK changed, or transient — " - "the key itself may still be valid)", - ) - else: - check_ok( - "Mem0 connected", - f"user_id={mem0_user} agent_id={mem0_agent}", - ) - except ImportError: - _fail_and_issue( - "Mem0 plugin not loadable", - "pip install mem0ai", - "Mem0 is set as memory provider but mem0ai is not installed", - issues, - ) - except Exception as _e: - check_warn("Mem0 check failed", str(_e)) - else: - # Generic check for other memory providers (openviking, - # supermemory, hindsight, byterover, retaindb, holographic). - # Each provider's `is_available()` only checks env-vars / CLI - # presence — it does NOT probe backend reachability (#36). - # Until a `health_check()` ABC method lands, the most we can - # truthfully say is "config detected" — not "active". - try: - from plugins.memory import load_memory_provider - _provider = load_memory_provider(_active_memory_provider) - if _provider and _provider.is_available(): - check_ok( - f"{_active_memory_provider} provider configured") - check_info( - "is_available() reports True (env vars / CLI " - "present) — backend reachability not probed by " - "doctor. Honcho + Mem0 do real probes; other " - "providers wait on a `health_check()` ABC method. " - "See TechDevGroup/hermes-agent#36.") - elif _provider: - check_warn( - f"{_active_memory_provider} configured but not available", - "run: hermes memory status") - else: - check_warn( - f"{_active_memory_provider} plugin not found", - "run: hermes memory setup") - except Exception as _e: - check_warn( - f"{_active_memory_provider} check failed", str(_e)) + # Unknown reason — surface verbatim. Better than + # swallowing. + check_warn(f"{_pname} health check failed", + _reason[:200]) _check_devagentic_graph() _check_cron_scheduler() diff --git a/plugins/memory/holographic/__init__.py b/plugins/memory/holographic/__init__.py index 681ce7660ce9..83b87ea544a1 100644 --- a/plugins/memory/holographic/__init__.py +++ b/plugins/memory/holographic/__init__.py @@ -128,6 +128,48 @@ def name(self) -> str: def is_available(self) -> bool: return True # SQLite is always available, numpy is optional + def health_check(self) -> tuple[bool, str]: + """Probe holographic by verifying the db_path's parent + directory is writable. There's no remote service to ping, + but a read-only HERMES_HOME (RO mount, wrong perms) would + still break the provider at runtime — so the probe is + meaningful. + + Returns (RFC #42 conventions): + (True, "") — parent writable. + (False, "unreachable: ") — mkdir failed or not writable. + (False, "config_error: ") — config load itself raised. + + MUST NOT raise. + """ + from pathlib import Path as _Path + import os as _os + try: + from hermes_constants import get_hermes_home + hermes_home = str(get_hermes_home()) + except Exception as exc: # noqa: BLE001 + return (False, f"config_error: {exc}") + + try: + db_path = str(self._config.get( + "db_path", f"{hermes_home}/memory_store.db")) + db_path = db_path.replace("$HERMES_HOME", hermes_home) + db_path = db_path.replace("${HERMES_HOME}", hermes_home) + parent = _Path(db_path).parent + try: + parent.mkdir(parents=True, exist_ok=True) + except OSError as exc: + return (False, + f"unreachable: db_path parent {parent} " + f"unwritable ({exc})") + if not _os.access(parent, _os.W_OK): + return (False, + f"unreachable: db_path parent {parent} " + "not writable (check filesystem permissions)") + except Exception as exc: # noqa: BLE001 + return (False, f"config_error: {exc}") + return (True, "") + def save_config(self, values, hermes_home): """Write config to config.yaml under plugins.hermes-memory-store.""" from pathlib import Path diff --git a/tests/hermes_cli/test_doctor.py b/tests/hermes_cli/test_doctor.py index 947e86335e71..c78362c62b78 100644 --- a/tests/hermes_cli/test_doctor.py +++ b/tests/hermes_cli/test_doctor.py @@ -418,18 +418,17 @@ def test_mem0_provider_not_installed_shows_fail(self, monkeypatch, tmp_path): assert "Memory Provider" in out assert "Built-in memory active" not in out - def test_generic_provider_wording_no_longer_implies_reachability( + def test_generic_provider_dispatches_to_health_check( self, monkeypatch, tmp_path): - """#36: generic providers (e.g. openviking) only check env-var - presence in is_available(). Doctor must not say "active" - (implies reachable); must say "configured" + flag the gap.""" - # Stub load_memory_provider to return a SimpleNamespace whose - # is_available() returns True without any network call — - # mirrors the production openviking/supermemory/etc. shape. + """#42 step 3: doctor's unified memory-provider dispatch + now calls health_check() on every provider — no per-provider + elif blocks. A provider returning (True, "") shows up as + " reachable" in the output.""" from types import SimpleNamespace fake = SimpleNamespace( is_available=lambda: True, + health_check=lambda: (True, ""), name=lambda: "openviking", ) import plugins.memory as _mem_pkg @@ -437,12 +436,51 @@ def test_generic_provider_wording_no_longer_implies_reachability( lambda name: fake) out = self._run_doctor_and_capture(monkeypatch, tmp_path, provider="openviking") - assert "openviking provider configured" in out + assert "openviking reachable" in out + # The pre-#42-step-3 wording is gone. assert "openviking provider active" not in out - # The info row must point at the open issue so operators can - # follow the design discussion if they care. - assert "#36" in out - assert "backend reachability not probed" in out + assert "openviking provider configured" not in out + assert "backend reachability not probed" not in out + + def test_generic_provider_dispatches_auth_failure_as_fail( + self, monkeypatch, tmp_path): + """#42 step 3: when a provider returns a "auth:" reason, + doctor maps it to _fail_and_issue (red row + issue list + entry), not check_warn.""" + from types import SimpleNamespace + + fake = SimpleNamespace( + is_available=lambda: True, + health_check=lambda: (False, "auth: 401 invalid key"), + name=lambda: "mem0", + ) + import plugins.memory as _mem_pkg + monkeypatch.setattr(_mem_pkg, "load_memory_provider", + lambda name: fake) + out = self._run_doctor_and_capture(monkeypatch, tmp_path, + provider="mem0") + assert "mem0 auth rejected" in out + assert "401 invalid key" in out + + def test_generic_provider_dispatches_unreachable_as_warn( + self, monkeypatch, tmp_path): + """unreachable: prefix → check_warn (transient, key may + still be valid).""" + from types import SimpleNamespace + + fake = SimpleNamespace( + is_available=lambda: True, + health_check=lambda: (False, "unreachable: connection refused"), + name=lambda: "honcho", + ) + import plugins.memory as _mem_pkg + monkeypatch.setattr(_mem_pkg, "load_memory_provider", + lambda name: fake) + out = self._run_doctor_and_capture(monkeypatch, tmp_path, + provider="honcho") + assert "honcho unreachable" in out + assert "connection refused" in out + assert "credentials may still be valid" in out def test_run_doctor_termux_treats_docker_and_browser_warnings_as_expected(monkeypatch, tmp_path): diff --git a/tests/plugins/memory/test_holographic_health_check.py b/tests/plugins/memory/test_holographic_health_check.py new file mode 100644 index 000000000000..2185c1b2f66c --- /dev/null +++ b/tests/plugins/memory/test_holographic_health_check.py @@ -0,0 +1,97 @@ +"""Tests for ``HolographicMemoryProvider.health_check`` (#42 step 2h). + +Override verifies db_path's parent directory is writable. Unlike +the cloud-backed providers, there's no remote service to ping — +but a read-only HERMES_HOME (RO mount, wrong perms) would still +break the provider at runtime, so the probe is meaningful. +""" +from __future__ import annotations + +from pathlib import Path + +import pytest + +from plugins.memory.holographic import HolographicMemoryProvider + + +def _provider(config: dict | None = None) -> HolographicMemoryProvider: + return HolographicMemoryProvider(config=config or {}) + + +def test_returns_true_when_parent_writable(tmp_path, monkeypatch): + db_path = tmp_path / "memory_store.db" + healthy, reason = _provider({"db_path": str(db_path)}).health_check() + assert healthy is True + assert reason == "" + + +def test_creates_missing_parent_directory(tmp_path, monkeypatch): + """When the configured db_path's parent doesn't yet exist, + health_check should mkdir it (parents=True) and report healthy.""" + db_path = tmp_path / "subdir" / "memory_store.db" + assert not db_path.parent.exists() + healthy, reason = _provider({"db_path": str(db_path)}).health_check() + assert healthy is True + assert db_path.parent.is_dir() + + +def test_returns_unreachable_when_parent_not_writable( + tmp_path, monkeypatch): + """Simulate a read-only HERMES_HOME by patching os.access to + deny W_OK for the relevant path.""" + db_path = tmp_path / "memory_store.db" + import os as _os + real_access = _os.access + target = db_path.parent + + def _fake_access(path, mode): + # Deny write access on the configured parent specifically. + if mode & _os.W_OK and Path(path).resolve() == target.resolve(): + return False + return real_access(path, mode) + + monkeypatch.setattr(_os, "access", _fake_access) + healthy, reason = _provider({"db_path": str(db_path)}).health_check() + assert healthy is False + assert reason.startswith("unreachable:") + assert "not writable" in reason + + +def test_returns_unreachable_on_mkdir_failure(tmp_path, monkeypatch): + """When mkdir raises OSError (e.g. parent directory is a file + rather than a dir), surface as unreachable, not propagate.""" + blocker = tmp_path / "blocker" + blocker.write_text("i am a file, not a dir") + db_path = blocker / "memory_store.db" # parent is a file → OSError + healthy, reason = _provider({"db_path": str(db_path)}).health_check() + assert healthy is False + assert reason.startswith("unreachable:") + assert "unwritable" in reason + + +def test_config_error_when_hermes_constants_import_fails( + monkeypatch, tmp_path): + """RFC #42 contract: health_check MUST NOT raise. Even when + hermes_constants becomes unimportable between construction + and probe time, the method returns a tuple.""" + import sys + # Construct with a real config so _load_plugin_config doesn't fire. + prov = _provider({"db_path": str(tmp_path / "memory_store.db")}) + # Now break the hermes_constants import that health_check itself + # performs. + monkeypatch.setitem(sys.modules, "hermes_constants", None) + healthy, reason = prov.health_check() + assert healthy is False + assert reason.startswith("config_error:") + + +def test_hermes_home_expansion_works(tmp_path, monkeypatch): + """The legacy `${HERMES_HOME}` template in db_path values is + substituted before the writability check.""" + monkeypatch.setenv("HERMES_HOME", str(tmp_path)) + # The provider reads HERMES_HOME via get_hermes_home() which + # respects the env var — verify the template substitution path. + healthy, reason = _provider( + {"db_path": "${HERMES_HOME}/memory_store.db"}).health_check() + assert healthy is True + assert reason == ""