fix(telemetry): extension failure classifiers are inert; the contract rejects contradictory install rows
- `_publish_failure_class`, `probe_failure_class` and `exception_failure_class` read a raise site's `failure_class` with `inspect.getattr_static` (`tagged_failure_class`), and the MCP type walk falls back to `connect_failed`. A third-party exception whose `__getattr__` or property raised replaced the user's PluginOperationError with RuntimeError (they run at the raise site, collection on or off), and the metric row was dropped; it now reads `exception`. - `counter_dimensions_are_valid` adds the cross-field rule the per-field enums cannot state: failure_class in the kind's own set, `none` exactly on success, registry only on skill rows. Pre-split rows (no failure_class/registry) still validate. - A refused dependency consent returns its reason as `_ConsentRefusal`, a str carrying the closed class (`non_interactive`, `deps_declined`, `manifest_invalid`), so publication no longer classifies by matching the user-facing copy. The text the user reads is unchanged. - `extension_registry` imports the hub's `_SOURCE_ID_ALIASES` instead of a hand copy. - test file: schema read uses utf-8-sig (windows-footguns).
This commit is contained in:
Teknium
@@ -796,18 +796,23 @@ def _probe_failure_reason(exc: BaseException) -> str:
|
||||
|
||||
def probe_failure_class(exc: BaseException) -> str:
|
||||
"""Extension-install class for a failed MCP probe, from the exception TYPE (the same tests
|
||||
:func:`_probe_failure_next_step` uses), never its message."""
|
||||
tagged = getattr(exc, "failure_class", None)
|
||||
if isinstance(tagged, str) and tagged:
|
||||
:func:`_probe_failure_next_step` uses), never its message. Never raises: an exception whose own
|
||||
attribute hooks raise reads ``connect_failed`` instead of replacing the user's error."""
|
||||
from hermes_cli.observability.shared_metrics_fields import tagged_failure_class
|
||||
|
||||
if tagged := tagged_failure_class(exc):
|
||||
return tagged
|
||||
from tools.mcp_tool_errors import _is_auth_error, _iter_exception_nodes, _unwrap_exception_group
|
||||
from tools.mcp_tool_node_abi import NodeAbiMismatchError
|
||||
root = _unwrap_exception_group(exc)
|
||||
if _is_auth_error(root) or getattr(getattr(root, "response", None), "status_code", None) in (401, 403):
|
||||
return "auth_required"
|
||||
# A missing stdio command (FileNotFoundError) or a native module built for another Node.
|
||||
if any(isinstance(node, (FileNotFoundError, NodeAbiMismatchError)) for node in _iter_exception_nodes(exc)):
|
||||
return "server_start_failed"
|
||||
try:
|
||||
root = _unwrap_exception_group(exc)
|
||||
if _is_auth_error(root) or getattr(getattr(root, "response", None), "status_code", None) in (401, 403):
|
||||
return "auth_required"
|
||||
# A missing stdio command (FileNotFoundError) or a native module built for another Node.
|
||||
if any(isinstance(node, (FileNotFoundError, NodeAbiMismatchError)) for node in _iter_exception_nodes(exc)):
|
||||
return "server_start_failed"
|
||||
except Exception: # a hook on the exception raised; the type tests could not finish
|
||||
logger.debug("MCP probe failure not classified", exc_info=True)
|
||||
return "connect_failed"
|
||||
|
||||
|
||||
|
||||
@@ -1151,7 +1151,22 @@ def counter_dimensions_are_valid(metric_name: str, dimensions: dict[str, Any]) -
|
||||
else value in contract[field]
|
||||
)
|
||||
for field in fields
|
||||
) and _extension_install_fields_agree(metric_name, dimensions)
|
||||
|
||||
|
||||
# ---- iuf c1 ----
|
||||
def _extension_install_fields_agree(metric_name: str, dimensions: dict[str, Any]) -> bool:
|
||||
"""Cross-field rule the per-field enums cannot state: an install row's ``failure_class`` is in
|
||||
its kind's set and is ``none`` exactly on success; only skill rows name a registry."""
|
||||
if metric_name != EXTENSION_INSTALL_METRIC or "failure_class" not in dimensions:
|
||||
return True # other metrics, and the pre-split shape without failure_class/registry
|
||||
kind, failure_class = dimensions["kind"], dimensions["failure_class"]
|
||||
return (
|
||||
failure_class in EXTENSION_KIND_FAILURE_CLASSES[kind]
|
||||
and (failure_class == "none") == (dimensions["outcome"] == "success")
|
||||
and (kind == "skill" or dimensions["registry"] == "none")
|
||||
)
|
||||
# ---- end iuf c1 ----
|
||||
|
||||
|
||||
def _relay_metadata(
|
||||
|
||||
@@ -200,14 +200,23 @@ def extension_install_fields(
|
||||
# ---- iuf c1 ----
|
||||
# Exceptions from these top-level packages are transport failures, whatever their class name.
|
||||
_NETWORK_MODULES = frozenset({"aiohttp", "h11", "h2", "httpcore", "httpx", "requests", "ssl", "urllib3", "websockets"})
|
||||
_SOURCE_ID_ALIASES = {"skills.sh": "skills-sh"} # = tools.skills_hub_install._SOURCE_ID_ALIASES
|
||||
|
||||
|
||||
def tagged_failure_class(error: object) -> str | None:
|
||||
"""The ``failure_class`` a raise site set, read statically: an attribute hook on a third-party
|
||||
exception must not raise from a classifier and replace the user's error."""
|
||||
import inspect
|
||||
|
||||
tagged = inspect.getattr_static(error, "failure_class", None)
|
||||
return tagged if isinstance(tagged, str) and tagged else None
|
||||
|
||||
|
||||
def exception_failure_class(error: BaseException) -> str:
|
||||
"""Why a raised install failed, from the exception alone: the class its raise site tagged
|
||||
(``failure_class`` attribute), else its TYPE. Never reads the message."""
|
||||
tagged = getattr(error, "failure_class", None)
|
||||
if isinstance(tagged, str) and tagged:
|
||||
(``failure_class`` attribute), else its TYPE. Never reads the message, and never runs the
|
||||
exception's own attribute hooks (a raising ``__getattr__`` or property reads by type)."""
|
||||
tagged = tagged_failure_class(error)
|
||||
if tagged:
|
||||
return tagged
|
||||
import socket
|
||||
import urllib.error
|
||||
@@ -238,6 +247,9 @@ def extension_registry(kind: str, registry: Any) -> str:
|
||||
"""The skills-hub adapter id for a skill row; plugin and MCP rows are ``none``."""
|
||||
if kind != "skill":
|
||||
return "none"
|
||||
# The hub's own alias table (`--source skills.sh`); already imported by the install it records.
|
||||
from tools.skills_hub_install import _SOURCE_ID_ALIASES
|
||||
|
||||
value = _norm(registry) or "none"
|
||||
value = _SOURCE_ID_ALIASES.get(value, value)
|
||||
return value if value in contract.EXTENSION_REGISTRIES else "other"
|
||||
|
||||
@@ -55,6 +55,16 @@ def _pc():
|
||||
return plugins_cmd
|
||||
|
||||
|
||||
class _ConsentRefusal(str):
|
||||
"""A refusal reason (user-facing text) carrying its closed extension-install ``failure_class``,
|
||||
so publication classifies the refusal without matching the copy."""
|
||||
|
||||
def __new__(cls, text: str, failure_class: str) -> "_ConsentRefusal":
|
||||
refusal = super().__new__(cls, text)
|
||||
refusal.failure_class = failure_class
|
||||
return refusal
|
||||
|
||||
|
||||
def _install_plugin_python_deps(
|
||||
manifest: dict, target: Path, console, *, assume_yes: bool = False
|
||||
) -> tuple[bool, Optional[str]]:
|
||||
@@ -78,7 +88,7 @@ def _install_plugin_python_deps(
|
||||
declaration = read_python_declaration(target)
|
||||
deps = declaration.install_requirements
|
||||
except Exception as exc:
|
||||
return False, f"invalid Python dependency declaration: {exc}"
|
||||
return False, _ConsentRefusal(f"invalid Python dependency declaration: {exc}", "manifest_invalid")
|
||||
has_python = declaration.is_member
|
||||
has_package_json = (target / "package.json").is_file()
|
||||
if not has_python and not has_package_json:
|
||||
@@ -136,13 +146,13 @@ def _consent_python_deps(
|
||||
"[dim]Non-interactive install — skipping dependency install. "
|
||||
"Run `hermes plugins enable` when ready to prepare them.[/dim]\n"
|
||||
)
|
||||
return False, "dependency install skipped (non-interactive)"
|
||||
return False, _ConsentRefusal("dependency install skipped (non-interactive)", "non_interactive")
|
||||
if not _ask_yes_no(("python", plugin_name, deps), " Prepare these with Hermes through PM now? [y/N]: ", console):
|
||||
console.print(
|
||||
"[dim]Skipped — run `hermes plugins enable` when ready "
|
||||
"to prepare them.[/dim]\n"
|
||||
)
|
||||
return False, "dependency install declined"
|
||||
return False, _ConsentRefusal("dependency install declined", "deps_declined")
|
||||
|
||||
# Consent only — the python-deps resolution itself runs inside the ONE
|
||||
# admission transaction at enable-commit time (C13): env + config move
|
||||
@@ -474,8 +484,10 @@ def _publish_failure_class(exc: BaseException) -> str:
|
||||
keeps the class plugins_transaction tagged, a filesystem error keeps its kind, and everything
|
||||
else PM raised (InstallError, ResolutionConflict, AdmissionRefused, a worker error) is a
|
||||
dependency failure."""
|
||||
tagged = getattr(exc, "failure_class", None)
|
||||
if isinstance(exc, _pc().PluginOperationError) and isinstance(tagged, str) and tagged != "other":
|
||||
from hermes_cli.observability.shared_metrics_fields import tagged_failure_class
|
||||
|
||||
tagged = tagged_failure_class(exc) if isinstance(exc, _pc().PluginOperationError) else None
|
||||
if tagged and tagged != "other":
|
||||
return tagged
|
||||
if isinstance(exc, PermissionError):
|
||||
return "permission"
|
||||
|
||||
@@ -13,18 +13,6 @@ def recover_plugin_publication(project: Path, row: dict, journal: Path) -> None:
|
||||
_recover_plugin_publication(project, row, journal)
|
||||
|
||||
|
||||
# ---- iuf c1 ----
|
||||
def _consent_failure_class(reason: object) -> str:
|
||||
"""Extension-install class for a refused dependency consent. ``reason`` is one of the closed
|
||||
strings plugins_cmd_install._install_plugin_python_deps / _consent_python_deps return."""
|
||||
if reason == "dependency install skipped (non-interactive)":
|
||||
return "non_interactive"
|
||||
if reason == "dependency install declined":
|
||||
return "deps_declined"
|
||||
return "manifest_invalid" # "invalid Python dependency declaration: ..."
|
||||
# ---- end iuf c1 ----
|
||||
|
||||
|
||||
def publish_plugin(staged: Path, target: Path, old_metadata: dict, new_metadata: dict,
|
||||
*, target_digest: str | None = None, require_consent: bool = False,
|
||||
assume_consent: bool = False) -> None:
|
||||
@@ -47,8 +35,11 @@ def publish_plugin(staged: Path, target: Path, old_metadata: dict, new_metadata:
|
||||
if not consented:
|
||||
outcome = ("Reinstall declined: {}. The installed plugin and active environment are unchanged."
|
||||
if target.exists() else "Install declined: {}. Nothing was installed.")
|
||||
# The refusal carries its closed class (plugins_cmd_install._ConsentRefusal).
|
||||
from hermes_cli.observability.shared_metrics_fields import tagged_failure_class
|
||||
|
||||
raise plugins_cmd.PluginOperationError(
|
||||
outcome.format(reason), failure_class=_consent_failure_class(reason))
|
||||
outcome.format(reason), failure_class=tagged_failure_class(reason) or "other")
|
||||
|
||||
sync_venv(explicit=True, plugins=StagedUpdate({
|
||||
"staged": str(staged.resolve()), "target": str(target.absolute()),
|
||||
|
||||
@@ -17,7 +17,7 @@ _SCHEMA = Path(observability.__file__).parent / "schemas/hermes.shared_metrics.v
|
||||
|
||||
|
||||
def _schema_dimensions(metric: str) -> list[dict]:
|
||||
schema = json.loads(_SCHEMA.read_text(encoding="utf-8"))
|
||||
schema = json.loads(_SCHEMA.read_text(encoding="utf-8-sig"))
|
||||
(definition,) = (d for d in schema["$defs"].values() if d.get("properties", {}).get("name", {}).get("const") == metric)
|
||||
dims = definition["properties"]["dimensions"]
|
||||
return dims["oneOf"] if "oneOf" in dims else [dims]
|
||||
@@ -138,3 +138,57 @@ def test_update_failure_class_reads_only_receipt_fields(receipt, expected):
|
||||
run, _ = update_metrics.update_receipt_fields(receipt)
|
||||
assert run["failure_class"] == expected
|
||||
assert contract.counter_dimensions_are_valid(contract.UPDATE_RUN_METRIC, run)
|
||||
|
||||
|
||||
class _HostileError(Exception):
|
||||
"""A third-party exception whose attribute hooks raise (a PM worker, an SDK under -W error)."""
|
||||
|
||||
def __getattr__(self, name):
|
||||
raise RuntimeError("hostile getattr")
|
||||
|
||||
|
||||
def test_failure_classifiers_never_raise_on_an_exception_whose_attribute_hooks_raise():
|
||||
"""Invariant: classification is inert. The raise-site classifiers run outside the metrics guard
|
||||
(collection on or off), so a raising hook must not replace the user's error, and the metric row
|
||||
still records (as ``exception``) instead of being dropped."""
|
||||
from hermes_cli.mcp_config import probe_failure_class
|
||||
from hermes_cli.plugins_cmd_install import _publish_failure_class
|
||||
|
||||
assert _publish_failure_class(_HostileError("x")) == "deps_failed"
|
||||
assert probe_failure_class(_HostileError("x")) == "connect_failed"
|
||||
for kind in contract.EXTENSION_KINDS:
|
||||
dims = fields.extension_install_fields(kind=kind, source="url", name=None, outcome="failed", error=_HostileError("x"))
|
||||
assert dims["failure_class"] == "exception", kind
|
||||
assert contract.counter_dimensions_are_valid(contract.EXTENSION_INSTALL_METRIC, dims)
|
||||
|
||||
|
||||
def test_contract_rejects_contradictory_extension_install_rows():
|
||||
"""Invariant: the validator enforces what the per-field enums cannot: a kind's own failure set,
|
||||
``none`` exactly on success, a registry only on skill rows. Pre-split rows still validate."""
|
||||
ok = {"kind": "skill", "name": "custom", "outcome": "failed", "source": "hub", "failure_class": "ambiguous",
|
||||
"registry": "github"}
|
||||
assert contract.counter_dimensions_are_valid(contract.EXTENSION_INSTALL_METRIC, ok)
|
||||
for bad in ({"kind": "plugin", "registry": "none"}, {"outcome": "success"}, {"failure_class": "none"},
|
||||
{"kind": "mcp_server", "failure_class": "other"}):
|
||||
assert not contract.counter_dimensions_are_valid(contract.EXTENSION_INSTALL_METRIC, {**ok, **bad}), bad
|
||||
legacy = {key: ok[key] for key in ("kind", "name", "outcome", "source")}
|
||||
assert contract.counter_dimensions_are_valid(contract.EXTENSION_INSTALL_METRIC, legacy)
|
||||
|
||||
|
||||
def test_a_declined_dependency_consent_carries_its_class_not_its_copy(monkeypatch):
|
||||
"""The refusal reason names its closed class where it is produced, so rewording the copy cannot
|
||||
turn a decline into ``manifest_invalid``; the user still reads the same text."""
|
||||
from types import SimpleNamespace
|
||||
|
||||
from hermes_cli import plugins_cmd_install
|
||||
|
||||
console = SimpleNamespace(print=lambda *a, **k: None)
|
||||
monkeypatch.setattr("sys.stdin.isatty", lambda: False)
|
||||
consented, reason = plugins_cmd_install._consent_python_deps("p", ("dep",), console)
|
||||
assert (consented, reason) == (False, "dependency install skipped (non-interactive)")
|
||||
assert fields.tagged_failure_class(reason) == "non_interactive"
|
||||
monkeypatch.setattr("sys.stdin.isatty", lambda: True)
|
||||
monkeypatch.setattr("sys.stdout.isatty", lambda: True)
|
||||
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"
|
||||
|
||||
|
||||
@@ -505,7 +505,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. Never the identifier, URL, error text or path; `name` keeps its existing catalog-or-`custom` rule. |
|
||||
| `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. |
|
||||
<!-- ---- end iuf c1 ---- -->
|
||||
<!-- ---- iuf c2 ---- -->
|
||||
|
||||
Reference in New Issue
Block a user