diff --git a/hermes_cli/observability/shared_metrics_update.py b/hermes_cli/observability/shared_metrics_update.py index 6899aa4be1..e171e68cff 100644 --- a/hermes_cli/observability/shared_metrics_update.py +++ b/hermes_cli/observability/shared_metrics_update.py @@ -46,7 +46,8 @@ _DESKTOP_APPLY_MODES = { # ``finalize_pending_update_receipt`` stores ``f"{type(exc).__name__}: {exc}"`` as the stop reason of # a run that ended on an exception (main.cmd_update, update_completion._finish). Only the leading # type name is read, never the message; other stop reasons ("sys.exit(1)", "completion exited 1", -# "Windows gateway recovery failed: ...") do not start with a bare CamelCase name and a colon. +# "Windows gateway recovery failed: ...") do not start with a bare CamelCase name and a colon +# (the fixed prefixes Hermes writes are matched by _STOP_REASON_PREFIX_CLASSES). _EXCEPTION_STOP_REASON = re.compile(r"([A-Z][A-Za-z0-9_]*):(?: |$)") # pm's own failure types (pm/package.py, pm/environment.py, pm/downloader.py, pm/lock.py, ...). _PM_ERROR_TYPES = frozenset({ @@ -58,6 +59,15 @@ _OS_ERROR_TYPES = frozenset( name for name, value in vars(builtins).items() if isinstance(value, type) and issubclass(value, OSError)) # subprocess.SubprocessError and its subclasses that reached the command boundary uncaught. _SUBPROCESS_ERROR_TYPES = frozenset({"CalledProcessError", "SubprocessError", "TimeoutExpired"}) +# Exception type name -> class, first match wins; any other type reads ``exception``. +_EXCEPTION_TYPE_CLASSES = ( + (_PM_ERROR_TYPES, "deps_failed"), (_OS_ERROR_TYPES, "os_error"), (_SUBPROCESS_ERROR_TYPES, "subprocess_failed"), +) +# Fixed stop-reason prefixes Hermes itself writes (only the prefix is read, never what follows): +# _update_takeover's preparation failure and update_completion's Windows gateway resume failure. +_STOP_REASON_PREFIX_CLASSES = ( + ("historical takeover preparation failed", "deps_failed"), ("Windows gateway recovery failed", "restart_failed"), +) # ---- end iuf c1 ---- @@ -113,14 +123,10 @@ def update_failure_class(receipt: dict[str, Any], stages: list[dict[str, Any]], return "fleet_unverified" # verification failed with no stale/down row and no failed restart if stages and stages[-1]["name"] == "restart" and stages[-1].get("outcome") == "skipped": return "restart_failed" # a skipped restart left the fleet owing one (completion exit 1) - if exc_type in _PM_ERROR_TYPES or reason.startswith("historical takeover preparation failed"): - return "deps_failed" - if exc_type in _OS_ERROR_TYPES: - return "os_error" - if exc_type in _SUBPROCESS_ERROR_TYPES: - return "subprocess_failed" + if by_reason := next((cls for prefix, cls in _STOP_REASON_PREFIX_CLASSES if reason.startswith(prefix)), None): + return by_reason if exc_type: - return "exception" + return next((cls for types, cls in _EXCEPTION_TYPE_CLASSES if exc_type in types), "exception") marked = {s["name"] for s in stages} if "apply" in marked: if "deps" not in marked: diff --git a/hermes_cli/skills_hub.py b/hermes_cli/skills_hub.py index 22180fd2df..be0bc8a27d 100644 --- a/hermes_cli/skills_hub.py +++ b/hermes_cli/skills_hub.py @@ -746,18 +746,28 @@ def _install_bundled(c: Console, name: str, invalidate_cache: bool) -> tuple: return SimpleNamespace(name=name, source="bundled"), "success", True -# Identifier prefixes each adapter writes (SkillMeta.identifier / _wrap_identifier), so a fetch no -# adapter answered still names the registry it was meant for. Bare owner/repo/path stays unresolved. -_REGISTRY_PREFIXES = ( - ("official/", "official"), ("skills-sh/", "skills-sh"), ("clawhub/", "clawhub"), ("lobehub/", "lobehub"), - ("browse-sh/", "browse-sh"), ("well-known:", "well-known"), ("http://", "url"), ("https://", "url"), -) +def _registry_prefixes() -> tuple: + """Identifier prefixes each adapter accepts, from the adapters themselves (``/``, the + skills.sh spellings, ``well-known:``), so a fetch no adapter answered still names the registry + it was meant for. Bare owner/repo/path stays unresolved.""" + from tools.skills_hub_clawhub import ClawHubSource + from tools.skills_hub_official import OptionalSkillSource + from tools.skills_hub_skillssh import SkillsShSource + from tools.skills_hub_sources import BrowseShSource, LobeHubSource, UrlSource, WellKnownSkillSource + + return ( + *((f"{cls.SOURCE_ID}/", cls.SOURCE_ID) for cls in (OptionalSkillSource, ClawHubSource, LobeHubSource, BrowseShSource)), + *((prefix, SkillsShSource.SOURCE_ID) for prefix in SkillsShSource._ID_PREFIX_ALIASES), + (f"{WellKnownSkillSource.SOURCE_ID}:", WellKnownSkillSource.SOURCE_ID), + ("http://", UrlSource.SOURCE_ID), ("https://", UrlSource.SOURCE_ID), + ) def _registry_from_identifier(identifier: str) -> str: - if identifier.startswith(("http://", "https://")) and "/.well-known/skills/" in identifier: + lowered = identifier.lower() + if lowered.startswith(("http://", "https://")) and "/.well-known/skills/" in lowered: return "well-known" - return next((sid for prefix, sid in _REGISTRY_PREFIXES if identifier.startswith(prefix)), "unresolved") + return next((sid for prefix, sid in _registry_prefixes() if lowered.startswith(prefix)), "unresolved") def _record_skill_install(identifier: str, bundle, outcome: str, attempt: Optional[dict] = None, @@ -846,7 +856,7 @@ def _install_skill(identifier: str, category: str, force: bool, c: Console, skip c.print(f"\n[bold]Fetching:[/] {identifier}") meta, bundle, _matched_source = _resolve_source_meta_and_bundle(identifier, sources) if _matched_source is not None: - attempt["registry"] = getattr(_matched_source, "SOURCE_ID", "") or "other" + attempt["registry"] = _matched_source.source_id() or "other" elif attempt.get("registry") == "unresolved": attempt["registry"] = _registry_from_identifier(identifier) if not bundle: diff --git a/tests/hermes_cli/test_shared_metrics_install_failures.py b/tests/hermes_cli/test_shared_metrics_install_failures.py index 08f7a07036..2cacab9f69 100644 --- a/tests/hermes_cli/test_shared_metrics_install_failures.py +++ b/tests/hermes_cli/test_shared_metrics_install_failures.py @@ -133,6 +133,8 @@ def test_a_partial_run_with_a_failed_stage_keeps_that_stage_and_its_verify_row_p stop_reason="completion exited 1"), "deps_failed"), (_receipt("failed", _ALL_PASSED[:4] + [("build", "failed"), ("restart", "success")], exit_code=1), "build_failed"), (_receipt("failed", [("plan", "success")], exit_code=1, stop_reason="AcmeError: secret"), "exception"), + (_receipt("failed", _ALL_PASSED, exit_code=1, stop_reason="Windows gateway recovery failed: PermissionError: x"), + "restart_failed"), ]) def test_update_failure_class_reads_only_receipt_fields(receipt, expected): run, _ = update_metrics.update_receipt_fields(receipt) @@ -192,3 +194,13 @@ def test_a_declined_dependency_consent_carries_its_class_not_its_copy(monkeypatc monkeypatch.setattr(plugins_cmd_install, "_ask_yes_no", lambda *a: False) assert fields.tagged_failure_class(plugins_cmd_install._consent_python_deps("p", ("dep",), console)[1]) == "deps_declined" + +@pytest.mark.parametrize(("identifier", "registry"), [ + ("skills.sh/acme/x", "skills-sh"), ("skils-sh/acme/x", "skills-sh"), ("OFFICIAL/x/y", "official"), + ("lobehub/x", "lobehub"), ("well-known:https://h/x", "well-known"), ("https://h/.well-known/skills/x", "well-known"), + ("https://h/a/SKILL.md", "url"), ("acme-corp/skills/x", "unresolved"), +]) +def test_an_unserved_identifier_names_the_registry_its_adapter_accepts(identifier, registry): + from hermes_cli.skills_hub import _registry_from_identifier + + assert _registry_from_identifier(identifier) == registry diff --git a/tests/hermes_cli/test_skills_failure_exit_code.py b/tests/hermes_cli/test_skills_failure_exit_code.py index fcc7c4b202..d0b80de0e7 100644 --- a/tests/hermes_cli/test_skills_failure_exit_code.py +++ b/tests/hermes_cli/test_skills_failure_exit_code.py @@ -32,7 +32,7 @@ def _scan_gate_env(monkeypatch, tmp_path, *, verdict="dangerous", installed=None monkeypatch.setattr(hub, "append_audit_log", lambda *a, **k: audit.append(a)) monkeypatch.setattr(hub, "HubLockFile", lambda: type( "Lock", (), {"get_installed": lambda self, n: installed})()) - monkeypatch.setattr(cli_hub, "_sources", lambda: [object()]) + monkeypatch.setattr(cli_hub, "_sources", lambda: [type("Source", (), {"source_id": lambda self: "skills-sh"})()]) monkeypatch.setattr(cli_hub, "_resolve_source_meta_and_bundle", lambda identifier, sources: (None, bundle, sources[0])) monkeypatch.setattr(cli_hub, "_record_skill_install", lambda *a, **k: None) diff --git a/tests/hermes_cli/test_skills_hub.py b/tests/hermes_cli/test_skills_hub.py index 265613d581..3891bb2b39 100644 --- a/tests/hermes_cli/test_skills_hub.py +++ b/tests/hermes_cli/test_skills_hub.py @@ -316,6 +316,9 @@ def test_inspect_reuses_one_ssrf_safe_client_for_metadata_and_bundle(monkeypatch return Response() class Source: + def source_id(self): + return "github" + def inspect(self, _identifier): hub._guarded_http_get("https://example.com/metadata") # The Hermes-index fetch must ride the same pool (no cache → real GET). diff --git a/tui_gateway/mcp_rpc_helpers.py b/tui_gateway/mcp_rpc_helpers.py index 192d3443ae..0f5eeea175 100644 --- a/tui_gateway/mcp_rpc_helpers.py +++ b/tui_gateway/mcp_rpc_helpers.py @@ -52,3 +52,13 @@ def summarize_server(name: str, cfg: dict, plugin: str | None = None) -> Dict[st "tools": cfg.get("tools"), "source": "plugin" if plugin is not None else "config", "plugin": plugin} + + +def record_mcp_add(entry: Any, server_config: Mapping[str, Any], saved: bool) -> None: + """Count an ``mcp.servers.add`` as an MCP extension install. A save fails only on a suspicious + command/args configuration (``_save_mcp_server`` returns False).""" + from hermes_cli.mcp_catalog import record_mcp_install + + source = "catalog" if entry is not None else ("url" if server_config.get("url") else "local") + record_mcp_install(source, entry.name if entry is not None else None, "success" if saved else "failed", + failure_class=None if saved else "config_rejected") diff --git a/tui_gateway/methods_tools.py b/tui_gateway/methods_tools.py index a3fb8962c8..9d4cd10e49 100644 --- a/tui_gateway/methods_tools.py +++ b/tui_gateway/methods_tools.py @@ -1608,8 +1608,7 @@ def _(rid, params: dict) -> dict: if bearer_token := params.get("bearer_token"): server_config["headers"] = mc._save_bearer_auth_token(name, str(bearer_token)) saved_ok = mc._save_mcp_server(name, server_config) - source = "catalog" if entry is not None else ("url" if server_config.get("url") else "local") - catalog.record_mcp_install(source, entry.name if entry else None, "success" if saved_ok else "failed", failure_class=None if saved_ok else "config_rejected") # noqa: E501 + _tools_mod("tui_gateway.mcp_rpc_helpers").record_mcp_add(entry, server_config, saved_ok) if not saved_ok: return _err(rid, 4001, f"server '{name}' rejected: suspicious command/args configuration") saved = mc._get_mcp_servers().get(name, server_config) diff --git a/website/docs/developer-guide/relay-shared-metrics.md b/website/docs/developer-guide/relay-shared-metrics.md index 72b61ab5ad..60620e92c4 100644 --- a/website/docs/developer-guide/relay-shared-metrics.md +++ b/website/docs/developer-guide/relay-shared-metrics.md @@ -506,7 +506,7 @@ they failed. Rows recorded before the change keep their old field set and still | Metric | Dimensions added | Question it answers | |---|---|---| | `hermes.extension.install.count` | failure class, registry | Why skill, plugin and MCP installs fail, and which skills-hub source fails. `failure_class` is `none` on success, else a closed name for the failure exit that stopped the install. Skills: `ambiguous`, `auth_rejected`, `fetch_failed`, `invalid_bundle`, `invalid_name`, `not_found`, `rate_limited`, `scan_blocked`, `stale_index`. Plugins: `already_installed`, `clone_failed`, `deps_declined`, `deps_failed`, `git_missing`, `incompatible`, `invalid_source`, `manifest_invalid`, `non_interactive`, `removed_from_catalog`, `scan_blocked`. MCP servers: `auth_required`, `bootstrap_failed`, `clone_failed`, `config_invalid`, `config_rejected`, `connect_failed`, `git_missing`, `missing_credentials`, `server_start_failed`. Every kind may also read `exception`, `filesystem_error`, `network`, `other`, `permission`. A raised error without a named class is classified by its Python type only (`PermissionError` reads `permission`, connection and timeout errors `network`, other `OSError`s `filesystem_error`, anything else `exception`, its type name dropped). `registry` is set on skill rows only: the skills-hub adapter that resolved or served the skill, one of `browse-sh`, `clawhub`, `github`, `hermes-index`, `lobehub`, `official`, `skills-sh`, `url`, `well-known` (the adapter ids `tools/skills_hub_search.py::create_source_router` builds), `unresolved` when no adapter answered, `none` for built-in restores and every plugin/MCP row, `other` otherwise. A row whose `failure_class` is outside its kind's set, is `none` on a failure or set on a success, or that names a registry on a plugin/MCP row is invalid. Never the identifier, URL, error text or path; `name` keeps its existing catalog-or-`custom` rule. | -| `hermes.update.run` | failure class | Why `hermes update` fails, next to where (`failed_stage`). `none` unless the outcome is `failed` or `refused`. Derived from the final update receipt only: `aborted_before_apply`, `build_failed`, `deps_failed`, `exception`, `fleet_stale`, `fleet_unverified`, `git_failed`, `interrupted`, `lock_held`, `managed_install`, `os_error`, `restart_failed`, `subprocess_failed`, else `other`; Desktop packaged self-updates report `unknown` (their RPC carries a stage, never a reason). An exception that ended the run is read by its type name alone, never its message. A failed run whose every stage mark passed now reads `failed_stage` `verify` (the post-restart verification wrote `partial`) or `restart` (a skipped restart left the fleet owing one) instead of `other`; a `partial` run with a failed build or restart mark keeps that stage, and its verify stage row is failed only when the fleet has a stale or down row. | +| `hermes.update.run` | failure class | Why `hermes update` fails, next to where (`failed_stage`). `none` unless the outcome is `failed` or `refused`. Derived from the final update receipt only: `aborted_before_apply`, `build_failed`, `deps_failed`, `exception`, `fleet_stale`, `fleet_unverified`, `git_failed`, `interrupted`, `lock_held`, `managed_install`, `os_error`, `restart_failed`, `subprocess_failed`, else `other`; Desktop packaged self-updates report `unknown` (their RPC carries a stage, never a reason). An exception that ended the run is read by its type name alone, never its message; a failed Windows gateway resume after the update reads `restart_failed`. A failed run whose every stage mark passed now reads `failed_stage` `verify` (the post-restart verification wrote `partial`) or `restart` (a skipped restart left the fleet owing one) instead of `other`; a `partial` run with a failed build or restart mark keeps that stage, and its verify stage row is failed only when the fleet has a stale or down row. | #### Fresh installs