diff --git a/plugins/memory/openviking/README.md b/plugins/memory/openviking/README.md index af3ed9b8a1..4ec4a23f6d 100644 --- a/plugins/memory/openviking/README.md +++ b/plugins/memory/openviking/README.md @@ -9,6 +9,12 @@ Context database by Volcengine (ByteDance) with filesystem-style knowledge hiera then `openviking-server doctor`) - OpenViking server running and reachable from Hermes +OpenViking 0.2.10 or newer is recommended. For backward compatibility, +Hermes can identify older servers that expose the legacy status-only health +response, but only when anonymous OpenAPI metadata also identifies the service +as OpenViking. OpenViking 0.2.6 and earlier are deprecated for this integration; +upgrade them to receive the current health contract and compatibility fixes. + ## Setup Prepare OpenViking first: diff --git a/plugins/memory/openviking/__init__.py b/plugins/memory/openviking/__init__.py index ff3793a0b8..123609e5e0 100644 --- a/plugins/memory/openviking/__init__.py +++ b/plugins/memory/openviking/__init__.py @@ -142,6 +142,20 @@ _LOCAL_SERVER_FAILED = "failed" _FAILED_CONFIG_RETRY_COOLDOWN_SECONDS = 30.0 _OPENVIKING_SERVER_LOG_RELATIVE_PATH = Path("logs") / "openviking-server.log" _OPENVIKING_RESPONDED_FAILURE_PREFIX = "OpenViking server responded" +_OPENVIKING_IDENTITY_MODERN = "modern" +_OPENVIKING_IDENTITY_LEGACY = "legacy" +_OPENVIKING_IDENTITY_UNHEALTHY = "unhealthy" +_OPENVIKING_IDENTITY_LEGACY_UNVERIFIED = "legacy-unverified" +_OPENVIKING_IDENTITY_INVALID = "invalid" +_OPENVIKING_IDENTIFIED_STATES = frozenset({ + _OPENVIKING_IDENTITY_MODERN, + _OPENVIKING_IDENTITY_LEGACY, +}) +_LEGACY_OPENVIKING_IDENTITY_DETAIL = ( + "returned OpenViking's legacy health response, but its anonymous " + "OpenAPI metadata did not identify OpenViking. If this is OpenViking 0.2.6 or " + "earlier, upgrade to OpenViking 0.2.10 or newer." +) _PENDING_SESSIONS_RELATIVE_DIR = Path("openviking") / "pending_sessions" _RUN_LOCKS_RELATIVE_DIR = Path("openviking") / "runs" _LEGACY_RECOVERY_LOCK_FILENAME = "legacy-recovery.lock" @@ -424,16 +438,24 @@ class _VikingClient: def health(self) -> bool: try: - return _is_openviking_health_payload(self.health_payload()) + identity, _health = _probe_openviking_identity(self) + return identity in _OPENVIKING_IDENTIFIED_STATES except Exception: return False - def health_payload(self) -> dict: + def _anonymous_json(self, path: str) -> dict: + """Probe server identity without disclosing credentials or tenant IDs.""" resp = self._httpx.get( - self._url("/health"), headers=self._headers(), timeout=3.0 + self._url(path), headers={"Accept": "application/json"}, timeout=3.0 ) return self._parse_response(resp) + def health_payload(self) -> dict: + return self._anonymous_json("/health") + + def openapi_payload(self) -> dict: + return self._anonymous_json("/openapi.json") + def validate_auth(self) -> dict: """Validate authenticated OpenViking access without mutating state.""" return self.get("/api/v1/system/status") @@ -844,7 +866,9 @@ def _normalize_openviking_url(url: str) -> str: if not trimmed: return _DEFAULT_ENDPOINT lower = trimmed.lower() - if lower in {"::1", "[::1]"}: + if lower in {"localhost", "127.0.0.1"}: + candidate = f"http://{trimmed}:1933" + elif lower in {"::1", "[::1]"}: candidate = "http://[::1]:1933" elif lower.startswith("[::1]:") or lower.startswith("::1:"): candidate = f"http://[::1]:{trimmed.rsplit(':', 1)[1]}" @@ -902,12 +926,57 @@ def _is_openviking_health_payload(payload: Any) -> bool: ) +def _is_legacy_openviking_health_payload(payload: Any) -> bool: + """Match the status-only health contract published through OpenViking 0.2.6.""" + return ( + isinstance(payload, dict) + and payload.get("status") == "ok" + and "healthy" not in payload + and "version" not in payload + ) + + +def _is_openviking_openapi_payload(payload: Any) -> bool: + if not isinstance(payload, dict): + return False + info = payload.get("info") + return isinstance(info, dict) and info.get("title") == "OpenViking API" + + +def _probe_openviking_identity(client: _VikingClient) -> tuple[str, Any]: + """Identify modern or legacy OpenViking before any authenticated request.""" + health = client.health_payload() + if isinstance(health, dict) and health.get("healthy") is False: + return _OPENVIKING_IDENTITY_UNHEALTHY, health + if _is_openviking_health_payload(health): + return _OPENVIKING_IDENTITY_MODERN, health + if not _is_legacy_openviking_health_payload(health): + return _OPENVIKING_IDENTITY_INVALID, health + + try: + openapi = client.openapi_payload() + except Exception: + logger.debug("Legacy OpenViking OpenAPI identity probe failed", exc_info=True) + return _OPENVIKING_IDENTITY_LEGACY_UNVERIFIED, health + if _is_openviking_openapi_payload(openapi): + return _OPENVIKING_IDENTITY_LEGACY, health + return _OPENVIKING_IDENTITY_LEGACY_UNVERIFIED, health + + +def _legacy_openviking_identity_error(subject: str) -> str: + return f"{subject} {_LEGACY_OPENVIKING_IDENTITY_DETAIL}" + + def _load_profile(path: Path, *, source: str, name: str) -> Optional[_OvcliProfile]: try: data = _load_ovcli_config(path) values = _connection_values_from_ovcli(data) except Exception as e: - logger.debug("Skipping invalid OpenViking CLI config %s: %s", path, e) + logger.warning( + "Skipping invalid OpenViking CLI config %s: %s", + path, + _format_openviking_exception(e), + ) return None return _OvcliProfile( source=source, @@ -1178,11 +1247,13 @@ def _validate_openviking_reachability(endpoint: str) -> tuple[bool, str]: try: client = _VikingClient(endpoint) if hasattr(client, "health_payload"): - payload = client.health_payload() - if isinstance(payload, dict) and payload.get("healthy") is False: + identity, _health = _probe_openviking_identity(client) + if identity == _OPENVIKING_IDENTITY_UNHEALTHY: return False, "OpenViking server responded but reported unhealthy status." - if _is_openviking_health_payload(payload): + if identity in _OPENVIKING_IDENTIFIED_STATES: return True, "" + if identity == _OPENVIKING_IDENTITY_LEGACY_UNVERIFIED: + return False, _legacy_openviking_identity_error("The server") return False, "OpenViking server responded, but its /health response is not valid OpenViking." elif client.health(): return True, "" @@ -1274,10 +1345,12 @@ def _validate_openviking_setup_values( user=_clean_config_value(values.get("user")), agent=_clean_config_value(values.get("agent")) or _DEFAULT_AGENT, ) - health = client.health_payload() - if isinstance(health, dict) and health.get("healthy") is False: + identity, health = _probe_openviking_identity(client) + if identity == _OPENVIKING_IDENTITY_UNHEALTHY: return False, "OpenViking server responded but reported unhealthy status.", None - if not _is_openviking_health_payload(health): + if identity == _OPENVIKING_IDENTITY_LEGACY_UNVERIFIED: + return False, _legacy_openviking_identity_error("The server"), None + if identity not in _OPENVIKING_IDENTIFIED_STATES: return False, "Server /health response is not valid OpenViking.", None if _should_probe_openviking_auth( health, @@ -1547,15 +1620,21 @@ def _classify_runtime_openviking_health(client: _VikingClient, endpoint: str) -> """Classify runtime health without treating every false result as server absence.""" try: if hasattr(client, "health_payload"): - payload = client.health_payload() - if isinstance(payload, dict) and payload.get("healthy") is False: + identity, _health = _probe_openviking_identity(client) + if identity == _OPENVIKING_IDENTITY_UNHEALTHY: return ( "responded", f"Service at {endpoint} responded but reported unhealthy OpenViking status." f"{_local_listener_suffix(endpoint)}", ) - if _is_openviking_health_payload(payload): + if identity in _OPENVIKING_IDENTIFIED_STATES: return "healthy", "" + if identity == _OPENVIKING_IDENTITY_LEGACY_UNVERIFIED: + return ( + "responded", + _legacy_openviking_identity_error(f"Service at {endpoint}") + + _local_listener_suffix(endpoint), + ) return ( "responded", f"Service at {endpoint} responded, but its /health response is not valid OpenViking." diff --git a/tests/plugins/memory/test_openviking_endpoint_always_blocked.py b/tests/plugins/memory/test_openviking_endpoint_always_blocked.py index 25b48ec820..729f3749d1 100644 --- a/tests/plugins/memory/test_openviking_endpoint_always_blocked.py +++ b/tests/plugins/memory/test_openviking_endpoint_always_blocked.py @@ -4,6 +4,7 @@ import pytest from plugins.memory.openviking import ( _OpenVikingEndpointError, + _local_openviking_bind, _normalize_openviking_url, _openviking_endpoint_is_always_blocked, ) @@ -18,6 +19,18 @@ def test_openviking_keeps_default_loopback(): assert _normalize_openviking_url("http://127.0.0.1:1933") == "http://127.0.0.1:1933" +@pytest.mark.parametrize("host", ["localhost", "127.0.0.1"]) +def test_openviking_bare_loopback_health_and_autostart_use_same_default_port(host): + endpoint = _normalize_openviking_url(host) + + assert endpoint == f"http://{host}:1933" + assert _local_openviking_bind(endpoint) == (host, 1933) + + +def test_openviking_explicit_loopback_url_preserves_implicit_http_port(): + assert _normalize_openviking_url("http://localhost") == "http://localhost" + + def test_openviking_blocks_ecs_metadata_hostname(): with pytest.raises(_OpenVikingEndpointError, match="blocked metadata address"): _normalize_openviking_url("http://metadata.google.internal/computeMetadata/v1/") diff --git a/tests/plugins/memory/test_openviking_provider.py b/tests/plugins/memory/test_openviking_provider.py index 52700c7a4a..e28c87f1a5 100644 --- a/tests/plugins/memory/test_openviking_provider.py +++ b/tests/plugins/memory/test_openviking_provider.py @@ -206,21 +206,25 @@ def test_linked_ovcli_without_url_falls_through_to_dashboard_endpoint(tmp_path, assert settings["api_key"] == "linked-key" -def test_profile_discovery_skips_unsafe_ovcli_endpoint(tmp_path): +def test_profile_discovery_warns_when_skipping_unsafe_ovcli_endpoint(tmp_path, caplog): profile_path = tmp_path / "ovcli.conf.blocked" profile_path.write_text( json.dumps({"url": "http://169.254.169.254/latest/meta-data"}), encoding="utf-8", ) - assert ( - openviking_module._load_profile( - profile_path, - source="saved", - name="blocked", + with caplog.at_level("WARNING", logger=openviking_module.__name__): + assert ( + openviking_module._load_profile( + profile_path, + source="saved", + name="blocked", + ) + is None ) - is None - ) + + assert "Skipping invalid OpenViking CLI config" in caplog.text + assert str(profile_path) in caplog.text def test_connection_values_omit_stale_identity_for_user_key_with_root_key(): @@ -736,6 +740,157 @@ def test_viking_client_delete_uses_identity_headers(monkeypatch): assert captured["kwargs"]["headers"]["X-OpenViking-Actor-Peer"] == "hermes" +def test_openviking_identity_probes_are_anonymous_before_authenticated_requests(monkeypatch): + calls = [] + + def response(payload): + return SimpleNamespace(status_code=200, text="", json=lambda: payload) + + def fake_get(url, **kwargs): + calls.append((url, kwargs["headers"])) + if url.endswith("/health"): + return response({"status": "ok"}) + if url.endswith("/openapi.json"): + return response({"info": {"title": "OpenViking API"}}) + if url.endswith("/api/v1/system/status"): + return response({"status": "ok"}) + if url.endswith("/api/v1/admin/accounts"): + return response({"status": "ok", "result": []}) + raise AssertionError(f"unexpected request: {url}") + + monkeypatch.setattr( + openviking_module, + "_get_httpx", + lambda: SimpleNamespace(get=fake_get), + ) + + valid, message, role = openviking_module._validate_openviking_setup_values({ + "endpoint": "https://openviking.example", + "api_key": "secret-key", + "account": "acct", + "user": "alice", + "agent": "hermes", + }) + + assert (valid, message, role) == (True, "", "root") + assert [url.removeprefix("https://openviking.example") for url, _headers in calls] == [ + "/health", + "/openapi.json", + "/api/v1/system/status", + "/api/v1/admin/accounts", + ] + assert calls[0][1] == {"Accept": "application/json"} + assert calls[1][1] == {"Accept": "application/json"} + for _url, headers in calls[2:]: + assert headers["X-API-Key"] == "secret-key" + assert headers["Authorization"] == "Bearer secret-key" + + +def test_repeated_openviking_health_probes_never_send_identity_headers(monkeypatch): + captured_headers = [] + client = _VikingClient( + "https://openviking.example", + api_key="secret-key", + account="acct", + user="alice", + agent="hermes", + ) + + def fake_get(_url, **kwargs): + captured_headers.append(kwargs["headers"]) + return SimpleNamespace( + status_code=200, + text="", + json=lambda: {"status": "ok", "healthy": True, "version": "0.2.10"}, + ) + + monkeypatch.setattr(client._httpx, "get", fake_get) + + assert client.health() is True + assert client.health() is True + assert captured_headers == [ + {"Accept": "application/json"}, + {"Accept": "application/json"}, + ] + + +def test_modern_openviking_identity_does_not_probe_openapi(): + client = MagicMock() + client.health_payload.return_value = { + "status": "ok", + "healthy": True, + "version": "0.2.10", + } + + state, health = openviking_module._probe_openviking_identity(client) + + assert state == "modern" + assert health["version"] == "0.2.10" + client.openapi_payload.assert_not_called() + + +def test_legacy_health_requires_openviking_openapi_identity_before_auth(monkeypatch): + events = [] + + class ForeignServiceClient: + def __init__(self, *args, **kwargs): + pass + + def health_payload(self): + events.append("health") + return {"status": "ok"} + + def openapi_payload(self): + events.append("openapi") + return {"info": {"title": "Unrelated Service"}} + + def validate_auth(self): + raise AssertionError("credentials must not be sent before identity is verified") + + monkeypatch.setattr(openviking_module, "_VikingClient", ForeignServiceClient) + + valid, message, role = openviking_module._validate_openviking_setup_values({ + "endpoint": "https://foreign.example", + "api_key": "secret-key", + }) + + assert valid is False + assert role is None + assert "0.2.6 or earlier" in message + assert "0.2.10 or newer" in message + assert events == ["health", "openapi"] + + +def test_verified_legacy_openviking_is_healthy_for_reachability_and_runtime(monkeypatch): + events = [] + + class LegacyOpenVikingClient: + def __init__(self, *args, **kwargs): + pass + + def health_payload(self): + events.append("health") + return {"status": "ok"} + + def openapi_payload(self): + events.append("openapi") + return {"info": {"title": "OpenViking API"}} + + monkeypatch.setattr(openviking_module, "_VikingClient", LegacyOpenVikingClient) + + reachable, message = openviking_module._validate_openviking_reachability( + "https://legacy.example" + ) + runtime_state, runtime_message = openviking_module._classify_runtime_openviking_health( + LegacyOpenVikingClient(), + "https://legacy.example", + ) + + assert (reachable, message) == (True, "") + assert (runtime_state, runtime_message) == ("healthy", "") + assert events == ["health", "openapi", "health", "openapi"] + + def test_validate_openviking_reachability_uses_health_only(monkeypatch): events = []