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.
This commit is contained in:
@@ -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.
|
||||
|
||||
@@ -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
|
||||
|
||||
@@ -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}'")
|
||||
|
||||
@@ -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
|
||||
|
||||
|
||||
|
||||
Reference in New Issue
Block a user