From acabfabf41dcbcb2a0a55f1b74ccc8493458e997 Mon Sep 17 00:00:00 2001 From: Teknium <127238744+teknium1@users.noreply.github.com> Date: Fri, 9 Oct 2026 01:42:01 -0700 Subject: [PATCH] fix(mcp): a failed OAuth catalog install from a card or the agent says why, instead of `exception` The connection-card / manage_connections install of an OAuth catalog entry runs the probe on a worker thread; `mcp_oauth.start` then rebuilt the worker's error as a bare RuntimeError from its text, dropping the `failure_class` `_probe_single_server` had tagged. Every pre-authorization failure (empty or schemeless URL, unreachable host, 404 endpoint, refused client registration) was recorded as `exception:RuntimeError` -> `exception`. - run_worker classifies the original exception with probe_failure_class and stores the class on the flow; start() re-tags the RuntimeError it raises with it. - probe_failure_class reads InvalidMcpUrlError (not http(s)://host) as config_invalid. - _require_rendered_remote tags an unset ${VAR} in url/headers as missing_credentials. - the "server responded but no token" refusal is auth_required. - a connect_failed message that does not name the server gets a one-line next step ("Could not connect to 'n8n-official' (Not Found). Check its URL ..."). n8n-official is the only catalog entry whose URL is a user-supplied `${VAR}`, so it is the only one where these user-side errors happen before authorization. --- hermes_cli/mcp_config.py | 4 +++- .../test_shared_metrics_install_failures.py | 17 +++++++++++++++++ tools/connectors/mcp_oauth.py | 14 ++++++++++++-- tools/mcp_tool_config.py | 6 ++++-- 4 files changed, 36 insertions(+), 5 deletions(-) diff --git a/hermes_cli/mcp_config.py b/hermes_cli/mcp_config.py index 58e1bb3911..d46c259c4c 100644 --- a/hermes_cli/mcp_config.py +++ b/hermes_cli/mcp_config.py @@ -802,10 +802,12 @@ def probe_failure_class(exc: BaseException) -> str: 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_errors import InvalidMcpUrlError, _is_auth_error, _iter_exception_nodes, _unwrap_exception_group from tools.mcp_tool_node_abi import NodeAbiMismatchError try: root = _unwrap_exception_group(exc) + if isinstance(root, InvalidMcpUrlError): # the configured URL is not http(s)://host + return "config_invalid" 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. diff --git a/tests/hermes_cli/test_shared_metrics_install_failures.py b/tests/hermes_cli/test_shared_metrics_install_failures.py index 1baca7587a..52ec57b7c4 100644 --- a/tests/hermes_cli/test_shared_metrics_install_failures.py +++ b/tests/hermes_cli/test_shared_metrics_install_failures.py @@ -225,3 +225,20 @@ def test_an_unserved_identifier_names_the_registry_its_adapter_accepts(identifie from hermes_cli.skills_hub import _registry_from_identifier assert _registry_from_identifier(identifier) == registry + + +@pytest.mark.parametrize(("url", "expected"), [ + ("n8n.example.com/mcp-server/http", "config_invalid"), # a pasted URL with no scheme + ("${HERMES_TEST_UNSET_MCP_URL}", "missing_credentials"), # the URL's setup value never arrived +]) +def test_an_oauth_card_install_that_fails_before_authorization_keeps_its_class(monkeypatch, url, expected): + """Invariant: the card/agent OAuth install (``mcp_oauth.start``) rebuilds the worker's error from + its text, so it must carry the worker's closed class; the row is never the bare ``exception``.""" + from tools.connectors import mcp_oauth + + monkeypatch.delenv("HERMES_TEST_UNSET_MCP_URL", raising=False) + with pytest.raises(Exception) as raised: + mcp_oauth.start("n8n-probe", cfg={"url": url, "auth": "oauth"}, url_timeout=60) + dims = fields.extension_install_fields(kind="mcp_server", source="catalog", name=None, outcome="failed", + error=raised.value) + assert dims["failure_class"] == expected diff --git a/tools/connectors/mcp_oauth.py b/tools/connectors/mcp_oauth.py index be4ad034f4..a98bb0510e 100644 --- a/tools/connectors/mcp_oauth.py +++ b/tools/connectors/mcp_oauth.py @@ -57,9 +57,11 @@ def probe_with_rollback( server_name, cfg, connect_timeout=login_connect_timeout(cfg), details=details) if not _oauth_tokens_present(server_name): details["initialized"] = False - raise RuntimeError( + no_token = RuntimeError( "The server responded, but no OAuth token was obtained — " "this provider may require a manually-registered OAuth client.") + no_token.failure_class = "auth_required" # type: ignore[attr-defined] + raise no_token except Exception as exc: if not details.get("initialized"): undo() @@ -178,6 +180,7 @@ def run_worker( if home_token is not None: reset_hermes_home_override(home_token) except Exception as exc: + from hermes_cli.mcp_config import probe_failure_class from tools.mcp_dashboard_oauth import exception_message msg = exception_message(exc) with suppress(Exception): @@ -185,7 +188,12 @@ def run_worker( msg = humanize_oauth_registration_error( server_name, exc, server_url=cfg.get("url") if isinstance(cfg, dict) else None ) or msg + failure_class = probe_failure_class(exc) + if failure_class == "connect_failed" and server_name not in msg: # e.g. a bare "Not Found" + msg = f"Could not connect to '{server_name}' ({msg}). Check its URL and that this machine can reach it." if flow is not None: + # Read by ``start``: its error is rebuilt from text, so the class rides on the flow. + flow.failure_class = failure_class flow.mark_error(msg) finally: if flow is not None: @@ -340,7 +348,9 @@ def start( auth_url=snapshot["authorization_url"], flow=flow, detail=_ssh_detail(flow.redirect_uri) if client_redirect_uri is None else "") if snapshot.get("status") == "error": - raise RuntimeError(snapshot.get("error") or "the OAuth flow failed before authorization") + failed = RuntimeError(snapshot.get("error") or "the OAuth flow failed before authorization") + failed.failure_class = getattr(flow, "failure_class", None) # type: ignore[attr-defined] + raise failed time.sleep(0.05) flow.mark_error("Timed out waiting for MCP authorization URL") raise TimeoutError(f"timed out waiting for the authorization URL for '{server_name}'") diff --git a/tools/mcp_tool_config.py b/tools/mcp_tool_config.py index 2d1434cd6b..85245b6429 100644 --- a/tools/mcp_tool_config.py +++ b/tools/mcp_tool_config.py @@ -472,8 +472,10 @@ def _require_rendered_remote(server_name: str, config: dict) -> dict: unresolved = sorted({m.group(1) for value in values for m in _ENV_VAR_PATTERN.finditer(str(value))}) if unresolved: refs = ", ".join(f"${{{ref}}}" for ref in unresolved) - raise ValueError(f"MCP server '{server_name}': {refs} in url/headers is not set in this profile's " - ".env or secret source") + unset = ValueError(f"MCP server '{server_name}': {refs} in url/headers is not set in this profile's " + ".env or secret source") + unset.failure_class = "missing_credentials" # type: ignore[attr-defined] + raise unset return config