fix(openviking): verify servers before sending credentials

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:

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."

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/")

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 = []