fix(plugin): align diagnostics and config route hygiene
This commit is contained in:
@@ -71,6 +71,7 @@ from __future__ import annotations
|
||||
import json
|
||||
import logging
|
||||
import uuid
|
||||
from urllib.parse import parse_qsl, urlencode, urlsplit, urlunsplit
|
||||
from typing import Any, Dict, List, Optional
|
||||
|
||||
logger = logging.getLogger(__name__)
|
||||
@@ -203,6 +204,49 @@ def _current_model_settings(config: Dict[str, Any]) -> Dict[str, Any]:
|
||||
return {"model": "", "provider": "", "api_mode": "", "base_url": ""}
|
||||
|
||||
|
||||
def _normalized_route_url(value: Any) -> str:
|
||||
raw = str(value or "").strip()
|
||||
if not raw:
|
||||
return ""
|
||||
try:
|
||||
parsed = urlsplit(raw)
|
||||
except ValueError:
|
||||
return raw.rstrip("/")
|
||||
scheme = parsed.scheme.lower()
|
||||
host = (parsed.hostname or "").lower()
|
||||
if not scheme or not host:
|
||||
return raw.rstrip("/")
|
||||
port = parsed.port
|
||||
netloc = host
|
||||
if port is not None and not (
|
||||
(scheme == "http" and port == 80) or
|
||||
(scheme == "https" and port == 443)
|
||||
):
|
||||
netloc = f"{host}:{port}"
|
||||
path = parsed.path.rstrip("/")
|
||||
query = urlencode(sorted(parse_qsl(parsed.query, keep_blank_values=True)))
|
||||
return urlunsplit((scheme, netloc, path, query, ""))
|
||||
|
||||
|
||||
def _model_route_identity(model_cfg: Dict[str, Any]) -> tuple[str, str, str]:
|
||||
return (
|
||||
str(model_cfg.get("default") or model_cfg.get("model") or "").strip(),
|
||||
str(model_cfg.get("provider") or "").strip().lower(),
|
||||
_normalized_route_url(model_cfg.get("base_url")),
|
||||
)
|
||||
|
||||
|
||||
def _maybe_clear_context_pin(
|
||||
original_model_cfg: Dict[str, Any],
|
||||
updated_model_cfg: Dict[str, Any],
|
||||
) -> None:
|
||||
"""Drop stale route-owned context pins when the configured route changes."""
|
||||
if "context_length" not in updated_model_cfg:
|
||||
return
|
||||
if _model_route_identity(original_model_cfg) != _model_route_identity(updated_model_cfg):
|
||||
updated_model_cfg.pop("context_length", None)
|
||||
|
||||
|
||||
def _parse_int(value: Any, default: int, minimum: int = 0) -> int:
|
||||
"""Parse an integer query parameter with bounds."""
|
||||
if value in (None, ""):
|
||||
@@ -621,10 +665,13 @@ def _make_config_handlers(adapter, upstream):
|
||||
model_cfg = config.get("model")
|
||||
if isinstance(model_cfg, dict):
|
||||
updated_model_cfg = dict(model_cfg)
|
||||
original_model_cfg = dict(model_cfg)
|
||||
elif isinstance(model_cfg, str) and model_cfg.strip():
|
||||
updated_model_cfg = {"default": model_cfg.strip()}
|
||||
original_model_cfg = {"default": model_cfg.strip()}
|
||||
else:
|
||||
updated_model_cfg = {}
|
||||
original_model_cfg = {}
|
||||
|
||||
if "model" in body:
|
||||
updated_model_cfg["default"] = str(body.get("model") or "").strip()
|
||||
@@ -632,6 +679,7 @@ def _make_config_handlers(adapter, upstream):
|
||||
updated_model_cfg["provider"] = str(body.get("provider") or "").strip()
|
||||
if "base_url" in body:
|
||||
updated_model_cfg["base_url"] = str(body.get("base_url") or "").strip()
|
||||
_maybe_clear_context_pin(original_model_cfg, updated_model_cfg)
|
||||
|
||||
config["model"] = updated_model_cfg
|
||||
try:
|
||||
|
||||
@@ -314,6 +314,100 @@ def _check(checks: list[dict[str, str]], check_id: str, status: str, summary: st
|
||||
checks.append({"id": check_id, "status": status, "summary": summary})
|
||||
|
||||
|
||||
def _component_status(raw: Any) -> str | None:
|
||||
if isinstance(raw, dict):
|
||||
for key in ("status", "state", "overall", "health"):
|
||||
value = raw.get(key)
|
||||
if isinstance(value, str) and value.strip():
|
||||
return value.strip().lower()
|
||||
if raw.get("ok") is True or raw.get("healthy") is True:
|
||||
return "ok"
|
||||
if raw.get("ok") is False or raw.get("healthy") is False:
|
||||
return "degraded"
|
||||
if isinstance(raw, str) and raw.strip():
|
||||
return raw.strip().lower()
|
||||
return None
|
||||
|
||||
|
||||
def _component_message(raw: Any) -> str | None:
|
||||
if not isinstance(raw, dict):
|
||||
return None
|
||||
for key in ("message", "summary", "error", "reason"):
|
||||
value = raw.get(key)
|
||||
if isinstance(value, str) and value.strip():
|
||||
return value.strip()
|
||||
return None
|
||||
|
||||
|
||||
def _sanitize_dashboard_components(status_body: Any) -> dict[str, Any]:
|
||||
"""Extract optional upstream /api/status component rollup for diagnostics.
|
||||
|
||||
Older Hermes builds omit the object entirely. The doctor treats absence as
|
||||
unsupported, not healthy or unhealthy, and only reports bounded text that is
|
||||
already sanitized by upstream.
|
||||
"""
|
||||
if not isinstance(status_body, dict):
|
||||
return {"supported": False, "overall": None, "components": {}}
|
||||
raw_components = status_body.get("components")
|
||||
if not isinstance(raw_components, dict):
|
||||
return {"supported": False, "overall": None, "components": {}}
|
||||
|
||||
components: dict[str, dict[str, Any]] = {}
|
||||
for name, raw in sorted(raw_components.items(), key=lambda item: str(item[0])):
|
||||
safe_name = str(name).strip()
|
||||
if not safe_name:
|
||||
continue
|
||||
component_status = _component_status(raw) or "unknown"
|
||||
item: dict[str, Any] = {"status": component_status}
|
||||
message = _component_message(raw)
|
||||
if message:
|
||||
item["message"] = message
|
||||
if isinstance(raw, dict):
|
||||
for key in (
|
||||
"configured",
|
||||
"connected",
|
||||
"healthy",
|
||||
"ok",
|
||||
"unhandled_5xx_count_5m",
|
||||
"self_test",
|
||||
):
|
||||
value = raw.get(key)
|
||||
if isinstance(value, (bool, int, float, str)) or value is None:
|
||||
item[key] = value
|
||||
components[safe_name] = item
|
||||
|
||||
overall = _component_status(status_body.get("overall"))
|
||||
if overall is None:
|
||||
statuses = {str(item["status"]).lower() for item in components.values()}
|
||||
overall = "ok" if statuses and statuses <= {"ok", "healthy", "ready"} else "degraded"
|
||||
return {"supported": True, "overall": overall, "components": components}
|
||||
|
||||
|
||||
def _component_check_status(component_health: dict[str, Any]) -> str:
|
||||
if not component_health.get("supported"):
|
||||
return "ok"
|
||||
overall = str(component_health.get("overall") or "").lower()
|
||||
if overall in {"ok", "healthy", "ready"}:
|
||||
return "ok"
|
||||
return "warn"
|
||||
|
||||
|
||||
def _component_check_summary(component_health: dict[str, Any]) -> str:
|
||||
if not component_health.get("supported"):
|
||||
return "dashboard component health rollup is not exposed by this Hermes build"
|
||||
components = component_health.get("components")
|
||||
if not isinstance(components, dict) or not components:
|
||||
return "dashboard component health rollup is present but empty"
|
||||
degraded = [
|
||||
f"{name}={data.get('status', 'unknown')}"
|
||||
for name, data in components.items()
|
||||
if str(data.get("status", "")).lower() not in {"ok", "healthy", "ready"}
|
||||
]
|
||||
if degraded:
|
||||
return "dashboard component health reports " + ", ".join(degraded)
|
||||
return "dashboard component health reports all components ready"
|
||||
|
||||
|
||||
def collect_doctor_report(
|
||||
*,
|
||||
api_url: str | None = None,
|
||||
@@ -429,6 +523,13 @@ def collect_doctor_report(
|
||||
)
|
||||
dashboard_json = dashboard_status.get("json")
|
||||
topology = dashboard_json if isinstance(dashboard_json, dict) else {}
|
||||
component_health = _sanitize_dashboard_components(topology)
|
||||
_check(
|
||||
checks,
|
||||
"dashboard-component-health",
|
||||
_component_check_status(component_health),
|
||||
_component_check_summary(component_health),
|
||||
)
|
||||
nous_state = topology.get("nous_session_valid")
|
||||
if nous_state == "terminal":
|
||||
_check(
|
||||
@@ -542,6 +643,7 @@ def collect_doctor_report(
|
||||
"api": {"capabilities": api_capabilities, "toolsets": api_toolsets},
|
||||
"dashboard": {
|
||||
"status": dashboard_status,
|
||||
"component_health": component_health,
|
||||
"audio_transcribe": dashboard_audio,
|
||||
"ws_ticket": dashboard_ws_ticket,
|
||||
"capabilities": dashboard_capabilities,
|
||||
|
||||
@@ -67,6 +67,7 @@ from __future__ import annotations
|
||||
import asyncio
|
||||
import json
|
||||
import logging
|
||||
from urllib.parse import parse_qsl, urlencode, urlsplit, urlunsplit
|
||||
from typing import Any, Dict
|
||||
|
||||
logger = logging.getLogger(__name__)
|
||||
@@ -237,6 +238,49 @@ def _current_model_settings(config: Dict[str, Any]) -> Dict[str, Any]:
|
||||
return {"model": "", "provider": "", "api_mode": "", "base_url": ""}
|
||||
|
||||
|
||||
def _normalized_route_url(value: Any) -> str:
|
||||
raw = str(value or "").strip()
|
||||
if not raw:
|
||||
return ""
|
||||
try:
|
||||
parsed = urlsplit(raw)
|
||||
except ValueError:
|
||||
return raw.rstrip("/")
|
||||
scheme = parsed.scheme.lower()
|
||||
host = (parsed.hostname or "").lower()
|
||||
if not scheme or not host:
|
||||
return raw.rstrip("/")
|
||||
port = parsed.port
|
||||
netloc = host
|
||||
if port is not None and not (
|
||||
(scheme == "http" and port == 80) or
|
||||
(scheme == "https" and port == 443)
|
||||
):
|
||||
netloc = f"{host}:{port}"
|
||||
path = parsed.path.rstrip("/")
|
||||
query = urlencode(sorted(parse_qsl(parsed.query, keep_blank_values=True)))
|
||||
return urlunsplit((scheme, netloc, path, query, ""))
|
||||
|
||||
|
||||
def _model_route_identity(model_cfg: Dict[str, Any]) -> tuple[str, str, str]:
|
||||
return (
|
||||
str(model_cfg.get("default") or model_cfg.get("model") or "").strip(),
|
||||
str(model_cfg.get("provider") or "").strip().lower(),
|
||||
_normalized_route_url(model_cfg.get("base_url")),
|
||||
)
|
||||
|
||||
|
||||
def _maybe_clear_context_pin(
|
||||
original_model_cfg: Dict[str, Any],
|
||||
updated_model_cfg: Dict[str, Any],
|
||||
) -> None:
|
||||
"""Drop stale route-owned context pins when the configured route changes."""
|
||||
if "context_length" not in updated_model_cfg:
|
||||
return
|
||||
if _model_route_identity(original_model_cfg) != _model_route_identity(updated_model_cfg):
|
||||
updated_model_cfg.pop("context_length", None)
|
||||
|
||||
|
||||
def _parse_int(value: Any, default: int, minimum: int = 0) -> int:
|
||||
"""Parse an integer query parameter with bounds."""
|
||||
if value in (None, ""):
|
||||
@@ -511,10 +555,13 @@ def _make_config_handlers(adapter, upstream):
|
||||
model_cfg = config.get("model")
|
||||
if isinstance(model_cfg, dict):
|
||||
updated_model_cfg = dict(model_cfg)
|
||||
original_model_cfg = dict(model_cfg)
|
||||
elif isinstance(model_cfg, str) and model_cfg.strip():
|
||||
updated_model_cfg = {"default": model_cfg.strip()}
|
||||
original_model_cfg = {"default": model_cfg.strip()}
|
||||
else:
|
||||
updated_model_cfg = {}
|
||||
original_model_cfg = {}
|
||||
|
||||
if "model" in body:
|
||||
updated_model_cfg["default"] = str(body.get("model") or "").strip()
|
||||
@@ -522,6 +569,7 @@ def _make_config_handlers(adapter, upstream):
|
||||
updated_model_cfg["provider"] = str(body.get("provider") or "").strip()
|
||||
if "base_url" in body:
|
||||
updated_model_cfg["base_url"] = str(body.get("base_url") or "").strip()
|
||||
_maybe_clear_context_pin(original_model_cfg, updated_model_cfg)
|
||||
|
||||
config["model"] = updated_model_cfg
|
||||
try:
|
||||
|
||||
@@ -186,5 +186,91 @@ class BootstrapMemoryBudgetTest(unittest.IsolatedAsyncioTestCase):
|
||||
store.add.assert_called_once_with("memory", "remember this")
|
||||
|
||||
|
||||
class BootstrapConfigRouteIdentityTest(unittest.IsolatedAsyncioTestCase):
|
||||
def setUp(self) -> None:
|
||||
_handlers._adapter_state.clear()
|
||||
|
||||
async def _patch_config(self, starting_config: dict, body: dict) -> dict:
|
||||
saved: list[dict] = []
|
||||
upstream = {
|
||||
"web": web,
|
||||
"load_config": mock.MagicMock(return_value=starting_config),
|
||||
"save_config": mock.MagicMock(side_effect=saved.append),
|
||||
"curated_models_for_provider": mock.MagicMock(return_value=[]),
|
||||
"list_available_providers": mock.MagicMock(return_value=[]),
|
||||
}
|
||||
handler = _handlers._make_config_handlers(_Adapter(), upstream)["update_config"]
|
||||
|
||||
response = await handler(_Request(body=body))
|
||||
|
||||
self.assertEqual(response.status, 200)
|
||||
self.assertEqual(len(saved), 1)
|
||||
return saved[0]["model"]
|
||||
|
||||
async def test_model_change_clears_stale_context_length(self) -> None:
|
||||
model = await self._patch_config(
|
||||
{
|
||||
"model": {
|
||||
"default": "old-model",
|
||||
"provider": "openrouter",
|
||||
"base_url": "https://openrouter.ai/api/v1",
|
||||
"context_length": 262144,
|
||||
"temperature": 0.2,
|
||||
}
|
||||
},
|
||||
{"model": "new-model"},
|
||||
)
|
||||
|
||||
self.assertEqual(model["default"], "new-model")
|
||||
self.assertEqual(model["temperature"], 0.2)
|
||||
self.assertNotIn("context_length", model)
|
||||
|
||||
async def test_provider_change_clears_stale_context_length(self) -> None:
|
||||
model = await self._patch_config(
|
||||
{
|
||||
"model": {
|
||||
"default": "same-model",
|
||||
"provider": "openrouter",
|
||||
"base_url": "https://openrouter.ai/api/v1",
|
||||
"context_length": 262144,
|
||||
}
|
||||
},
|
||||
{"provider": "anthropic"},
|
||||
)
|
||||
|
||||
self.assertEqual(model["provider"], "anthropic")
|
||||
self.assertNotIn("context_length", model)
|
||||
|
||||
async def test_equivalent_base_url_preserves_context_length(self) -> None:
|
||||
model = await self._patch_config(
|
||||
{
|
||||
"model": {
|
||||
"default": "same-model",
|
||||
"provider": "custom",
|
||||
"base_url": "https://Example.COM:443/v1/?b=2&a=1",
|
||||
"context_length": 8192,
|
||||
}
|
||||
},
|
||||
{"base_url": "https://example.com/v1?a=1&b=2"},
|
||||
)
|
||||
|
||||
self.assertEqual(model["context_length"], 8192)
|
||||
|
||||
async def test_different_base_url_path_clears_context_length(self) -> None:
|
||||
model = await self._patch_config(
|
||||
{
|
||||
"model": {
|
||||
"default": "same-model",
|
||||
"provider": "custom",
|
||||
"base_url": "https://example.com/v1",
|
||||
"context_length": 8192,
|
||||
}
|
||||
},
|
||||
{"base_url": "https://example.com/alternate"},
|
||||
)
|
||||
|
||||
self.assertNotIn("context_length", model)
|
||||
|
||||
|
||||
if __name__ == "__main__":
|
||||
unittest.main()
|
||||
|
||||
@@ -129,6 +129,52 @@ class DoctorTests(unittest.TestCase):
|
||||
self.assertEqual(checks["dashboard-nous-session"]["status"], "warn")
|
||||
self.assertIn("multiplex", checks["dashboard-topology"]["summary"])
|
||||
|
||||
def test_doctor_summarizes_dashboard_component_health(self) -> None:
|
||||
probe = _probe_map({
|
||||
"http://dash.example:9119/api/status": {
|
||||
"ok": True,
|
||||
"exists": True,
|
||||
"status": 200,
|
||||
"json": {
|
||||
"overall": "degraded",
|
||||
"components": {
|
||||
"gateway": {"status": "ok"},
|
||||
"storage": {"status": "degraded", "message": "state DB unavailable"},
|
||||
},
|
||||
},
|
||||
},
|
||||
})
|
||||
with mock.patch("plugin.doctor.assess_gateway_heartbeat", return_value={"status": "missing", "supported": False}):
|
||||
report = doctor.collect_doctor_report(
|
||||
dashboard_url="http://dash.example:9119", probe=probe, site_dirs=[]
|
||||
)
|
||||
|
||||
checks = {item["id"]: item for item in report["checks"]}
|
||||
self.assertEqual(checks["dashboard-component-health"]["status"], "warn")
|
||||
self.assertIn("storage=degraded", checks["dashboard-component-health"]["summary"])
|
||||
component_health = report["standard"]["dashboard"]["component_health"]
|
||||
self.assertTrue(component_health["supported"])
|
||||
self.assertEqual(component_health["components"]["storage"]["message"], "state DB unavailable")
|
||||
|
||||
def test_doctor_treats_missing_component_health_as_unsupported(self) -> None:
|
||||
probe = _probe_map({
|
||||
"http://dash.example:9119/api/status": {
|
||||
"ok": True,
|
||||
"exists": True,
|
||||
"status": 200,
|
||||
"json": {"nous_session_valid": True},
|
||||
},
|
||||
})
|
||||
with mock.patch("plugin.doctor.assess_gateway_heartbeat", return_value={"status": "missing", "supported": False}):
|
||||
report = doctor.collect_doctor_report(
|
||||
dashboard_url="http://dash.example:9119", probe=probe, site_dirs=[]
|
||||
)
|
||||
|
||||
checks = {item["id"]: item for item in report["checks"]}
|
||||
self.assertEqual(checks["dashboard-component-health"]["status"], "ok")
|
||||
self.assertIn("not exposed", checks["dashboard-component-health"]["summary"])
|
||||
self.assertFalse(report["standard"]["dashboard"]["component_health"]["supported"])
|
||||
|
||||
def test_doctor_ignores_non_object_dashboard_status_body(self) -> None:
|
||||
probe = _probe_map({
|
||||
"http://dash.example:9119/api/status": {
|
||||
|
||||
Reference in New Issue
Block a user