diff --git a/libs/arcade-core/arcade_core/executor.py b/libs/arcade-core/arcade_core/executor.py index 3589ba0b5..d271a4fe6 100644 --- a/libs/arcade-core/arcade_core/executor.py +++ b/libs/arcade-core/arcade_core/executor.py @@ -19,6 +19,61 @@ ) +def _render_declared_type(value_schema: Any) -> str: + """Render a parameter's declared type, e.g. ``string`` or ``array[string]``.""" + val_type = str(value_schema.val_type) + inner = getattr(value_schema, "inner_val_type", None) + if val_type == "array" and inner: + return f"array[{inner}]" + return val_type + + +def _expected_shape_guidance( + definition: ToolDefinition | None, + rejected_fields: list[str], +) -> str: + """Describe the declared shape of the parameters that were rejected. + + Built strictly from the tool's own ``ToolDefinition`` -- never from the + submitted values, which may contain secrets or PII (see the note in + ``_serialize_input``). Only the rejected parameters are described: the + caller already received the full schema from ``tools/list``, so echoing all + of it on every failure is noise that buries the actionable part. + + Returns an empty string when there is nothing useful to add, so callers can + append unconditionally. + """ + if definition is None or not rejected_fields: + return "" + + try: + parameters = {param.name: param for param in definition.input.parameters} + except AttributeError: + return "" + + lines: list[str] = [] + for name in rejected_fields: + param = parameters.get(name) + if param is None: + # A rejected key with no declared parameter (e.g. an unexpected + # extra argument) has no shape to describe. + continue + qualifier = "required" if param.required else "optional" + line = f" - {param.name} ({_render_declared_type(param.value_schema)}, {qualifier})" + if param.description: + line += f": {param.description}" + enum_values = getattr(param.value_schema, "enum", None) + if enum_values: + line += f" [allowed values: {', '.join(str(v) for v in enum_values)}]" + lines.append(line) + + if not lines: + return "" + + rendered = "\n".join(lines) + return f"Expected:\n{rendered}\n\nFix these arguments and call the tool again." + + class ToolExecutor: @staticmethod async def run( @@ -46,7 +101,7 @@ async def run( try: # serialize the input model - inputs = await ToolExecutor._serialize_input(input_model, **kwargs) + inputs = await ToolExecutor._serialize_input(input_model, definition, **kwargs) # prepare the arguments for the function call func_args = inputs.model_dump() @@ -90,9 +145,23 @@ async def run( ) @staticmethod - async def _serialize_input(input_model: type[BaseModel], **kwargs: Any) -> BaseModel: + async def _serialize_input( + input_model: type[BaseModel], + definition: ToolDefinition | None = None, + /, + **kwargs: Any, + ) -> BaseModel: """ Serialize the input to a tool function. + + ``input_model`` and ``definition`` are positional-only: ``**kwargs`` holds + the caller-supplied tool arguments, and a tool is free to declare a + parameter named ``definition`` (or ``input_model``). Positional-only + placement keeps such an argument in ``kwargs`` instead of colliding with + these parameters. + + ``definition`` is optional enrichment used to describe the expected shape + of rejected parameters; validation works without it. """ try: # TODO Logging and telemetry @@ -115,8 +184,24 @@ async def _serialize_input(input_model: type[BaseModel], **kwargs: Any) -> BaseM f"{'.'.join(str(loc) for loc in err['loc']) or ''}[{err['type']}]" for err in e.errors() ) + # Field paths of the rejected arguments, de-duplicated in the order + # Pydantic reported them. Only the top-level name is used, since that + # is what maps onto a declared tool parameter. + rejected_fields: list[str] = [] + for err in e.errors(): + if not err["loc"]: + continue + field = str(err["loc"][0]) + if field not in rejected_fields: + rejected_fields.append(field) + + message = f"Invalid input: {summary}" + guidance = _expected_shape_guidance(definition, rejected_fields) + if guidance: + message = f"{message}\n\n{guidance}" + raise ToolInputError( - message=f"Invalid input: {summary}", + message=message, developer_message=f"Pydantic validation failed: {developer_summary}", ) from e diff --git a/libs/arcade-core/pyproject.toml b/libs/arcade-core/pyproject.toml index 43658ec9d..15e356ac7 100644 --- a/libs/arcade-core/pyproject.toml +++ b/libs/arcade-core/pyproject.toml @@ -1,6 +1,6 @@ [project] name = "arcade-core" -version = "4.11.0" +version = "4.12.0" description = "Arcade Core - Core library for Arcade platform" readme = "README.md" license = { text = "MIT" } diff --git a/libs/arcade-mcp-server/arcade_mcp_server/server.py b/libs/arcade-mcp-server/arcade_mcp_server/server.py index f45585e0d..843165a21 100644 --- a/libs/arcade-mcp-server/arcade_mcp_server/server.py +++ b/libs/arcade-mcp-server/arcade_mcp_server/server.py @@ -1599,9 +1599,27 @@ async def _handle_call_tool( session and not session.has_feature("tool_execution") ): self._tracker.track_tool_call(False, "invalid tool input") + # Surface the curated user-facing message, never + # ``str(error)``: that renders the whole ToolCallError + # model, so ``developer_message`` and ``stacktrace`` + # would reach the client unconditionally. The stacktrace + # of an input-validation failure is a Pydantic traceback + # embedding ``input_value=``, i.e. the rejected argument + # itself — which the executor deliberately keeps out of + # the surfaced fields because it may hold secrets or PII. + # Route internals through the debug-flag gate instead, so + # this branch matches the 2025-11-25 one below. + legacy_message = error.message + if error.additional_prompt_content: + legacy_message += f"\n\n{error.additional_prompt_content}" + legacy_message = augment_error_message_for_debug( + legacy_message, + error.developer_message, + error.stacktrace, + ) return JSONRPCError( id=message.id, - error={"code": INVALID_PARAMS, "message": str(error)}, + error={"code": INVALID_PARAMS, "message": legacy_message}, ) error_text = error.message diff --git a/libs/arcade-mcp-server/pyproject.toml b/libs/arcade-mcp-server/pyproject.toml index c42392717..65cd8a716 100644 --- a/libs/arcade-mcp-server/pyproject.toml +++ b/libs/arcade-mcp-server/pyproject.toml @@ -4,7 +4,7 @@ build-backend = "hatchling.build" [project] name = "arcade-mcp-server" -version = "1.26.0" +version = "1.26.1" description = "Model Context Protocol (MCP) server framework for Arcade.dev" readme = "README.md" authors = [{ name = "Arcade.dev" }] diff --git a/libs/tests/arcade_mcp_server/test_invalid_input_legacy_protocol.py b/libs/tests/arcade_mcp_server/test_invalid_input_legacy_protocol.py new file mode 100644 index 000000000..5b42bc05a --- /dev/null +++ b/libs/tests/arcade_mcp_server/test_invalid_input_legacy_protocol.py @@ -0,0 +1,212 @@ +"""Tests for invalid-input errors on the MCP 2025-06-18 (legacy) tool-call path. + +Input validation failures are version-gated: 2025-11-25 clients get a +``CallToolResult(isError=True)``, 2025-06-18 clients get a JSON-RPC +``-32602``. The legacy branch used to serialize the whole ``ToolCallError`` +pydantic model with ``str(error)``, which dumps every field -- including +``developer_message`` and ``stacktrace``. + +That matters beyond cosmetics. Pydantic's traceback embeds ``input_value=``, +so the rejected argument was echoed back to the caller verbatim. The executor +deliberately keeps rejected values out of ``message``/``developer_message`` +because they may hold secrets or PII, and ``_debug_exposure`` exists so that +stacktraces reach a client only behind an explicit opt-in env flag. The legacy +branch bypassed both. +""" + +from typing import Annotated + +import pytest +import pytest_asyncio +from arcade_core.catalog import ToolCatalog +from arcade_mcp_server import _debug_exposure as debug_exposure +from arcade_mcp_server import tool +from arcade_mcp_server.server import MCPServer +from arcade_mcp_server.settings import MCPSettings +from arcade_mcp_server.types import CallToolRequest, CallToolResult, JSONRPCError, JSONRPCResponse + +# Read the flag names and the activation acknowledgement from the module that +# defines them rather than restating them. ``scripts/check_debug_leak_flags_off.py`` +# fails the build if the ack string appears in any tracked file outside its +# allowlist, and that guard is worth keeping narrow -- so this file must not +# contain a copy of it. +_LEAK_MAGIC = debug_exposure._DEBUG_LEAK_MAGIC +_ENV_STACKTRACE = debug_exposure._ENV_EXPOSE_STACKTRACE +_ENV_DEV_MSG = debug_exposure._ENV_EXPOSE_DEVELOPER_MESSAGE + +SENTINEL_ARG = "SENTINEL_ARG_VALUE_DO_NOT_LEAK_42" + + +@pytest.fixture(autouse=True) +def _reset_leak_state(monkeypatch): + monkeypatch.delenv(_ENV_DEV_MSG, raising=False) + monkeypatch.delenv(_ENV_STACKTRACE, raising=False) + debug_exposure._warned_rejected.clear() + debug_exposure._warned_activated.clear() + yield + debug_exposure._warned_rejected.clear() + debug_exposure._warned_activated.clear() + + +@tool +def needs_a_list( + tags: Annotated[list[str], "A list of tags"], +) -> Annotated[str, "Result"]: + """Tool whose argument fails validation when given a bare string.""" + return ",".join(tags) + + +class _FakeSession: + """Minimal stand-in for ServerSession with controllable protocol features.""" + + def __init__(self, features: set[str]) -> None: + self._features = features + self.session_id = "test-session" + # stdio keeps _check_transport_restrictions from short-circuiting the + # call before input validation runs. + self.init_options = {"transport_type": "stdio"} + + def has_feature(self, feature: str) -> bool: + return feature in self._features + + def has_capability(self, capability: str) -> bool: + return False + + +# Arcade derives the tool name from the function (``needs_a_list`` -> +# ``NeedsAList``), so resolve it from the catalog rather than hardcoding it -- +# a wrong literal here makes the server answer "Unknown tool" and every +# absence-based assertion below would pass vacuously. +_CATALOG = ToolCatalog() +_CATALOG.add_tool(needs_a_list, "LegacyToolkit") +TOOL_FQN = str(_CATALOG.find_tool_by_func(needs_a_list).get_fully_qualified_name()) + + +@pytest_asyncio.fixture +async def server(): + srv = MCPServer( + catalog=_CATALOG, + name="Legacy Input Server", + version="0.0.0", + settings=MCPSettings(), + ) + await srv.start() + try: + yield srv + finally: + await srv.stop() + + +async def _call_with_bad_input(srv, session): + return await srv._handle_call_tool( + CallToolRequest( + jsonrpc="2.0", + id=1, + method="tools/call", + params={"name": TOOL_FQN, "arguments": {"tags": SENTINEL_ARG}}, + ), + session=session, + ) + + +def _legacy_error_text(response) -> str: + assert isinstance(response, JSONRPCError), f"expected JSONRPCError, got {type(response)}" + error = response.error + text = error["message"] if isinstance(error, dict) else error.message + # Anchor: if the call never reached input validation (e.g. "Unknown tool"), + # the absence assertions in this module would pass for the wrong reason. + assert "Invalid input" in text, f"did not reach input validation; got: {text!r}" + return text + + +class TestLegacyPathDoesNotLeakInternals: + @pytest.mark.asyncio + async def test_rejected_argument_value_is_not_echoed(self, server): + """The reported class of bug: the rejected value must not reach the client.""" + response = await _call_with_bad_input(server, _FakeSession(set())) + assert SENTINEL_ARG not in _legacy_error_text(response) + + @pytest.mark.asyncio + async def test_stacktrace_is_not_exposed_without_the_flag(self, server): + text = _legacy_error_text(await _call_with_bad_input(server, _FakeSession(set()))) + assert "Traceback" not in text + assert "stacktrace=" not in text + + @pytest.mark.asyncio + async def test_model_repr_fields_are_not_dumped(self, server): + """``str(ToolCallError)`` renders ``field=value`` pairs; the client must + receive a message, not a model dump.""" + text = _legacy_error_text(await _call_with_bad_input(server, _FakeSession(set()))) + for leaked_field in ("kind=", "developer_message=", "can_retry=", "status_code="): + assert leaked_field not in text, f"{leaked_field!r} leaked into the client message" + + @pytest.mark.asyncio + async def test_message_is_still_actionable(self, server): + """Suppressing internals must not strip the useful part.""" + text = _legacy_error_text(await _call_with_bad_input(server, _FakeSession(set()))) + assert "Invalid input" in text + assert "tags" in text + + +class TestLegacyPathHonorsDebugFlags: + @pytest.mark.asyncio + async def test_stacktrace_appears_only_when_flag_is_set(self, server, monkeypatch): + """The escape hatch must still work on the legacy path -- the fix routes + through ``augment_error_message_for_debug`` rather than dropping it.""" + monkeypatch.setenv(_ENV_STACKTRACE, _LEAK_MAGIC) + text = _legacy_error_text(await _call_with_bad_input(server, _FakeSession(set()))) + assert "[DEBUG] stacktrace:" in text + + @pytest.mark.asyncio + async def test_developer_message_appears_only_when_flag_is_set(self, server, monkeypatch): + monkeypatch.setenv(_ENV_DEV_MSG, _LEAK_MAGIC) + text = _legacy_error_text(await _call_with_bad_input(server, _FakeSession(set()))) + assert "[DEBUG] developer_message:" in text + + +class TestModernPathUnchanged: + @pytest.mark.asyncio + async def test_modern_session_still_gets_call_tool_result(self, server): + """2025-11-25 clients keep the CallToolResult shape, and stay leak-free.""" + response = await _call_with_bad_input(server, _FakeSession({"tool_execution"})) + assert isinstance(response, JSONRPCResponse) + assert isinstance(response.result, CallToolResult) + assert response.result.isError is True + text = response.result.content[0].text + assert "Invalid input" in text + assert SENTINEL_ARG not in text + + +class TestLegacyPathKeepsRetryGuidance: + @pytest.mark.asyncio + async def test_additional_prompt_content_reaches_the_client(self, server): + """``additional_prompt_content`` is guidance authored for the caller, not + an internal — so suppressing internals must not drop it. The 2025-11-25 + branch appends it, and this branch has to match; a bad-input error does + not carry it today, so drive it through a crafted executor result.""" + from unittest.mock import patch + + from arcade_core.errors import ErrorKind + from arcade_core.schema import ToolCallError, ToolCallOutput + + crafted = ToolCallOutput( + error=ToolCallError( + message="Invalid input: tags: Input should be a valid list", + kind=ErrorKind.TOOL_RUNTIME_BAD_INPUT_VALUE, + developer_message="internal detail that must stay hidden", + additional_prompt_content="Pass tags as a JSON array, e.g. [\"a\", \"b\"].", + stacktrace="Traceback (most recent call last): ...", + status_code=400, + ) + ) + + with patch( + "arcade_mcp_server.server.ToolExecutor.run", + return_value=crafted, + ): + text = _legacy_error_text(await _call_with_bad_input(server, _FakeSession(set()))) + + assert 'Pass tags as a JSON array, e.g. ["a", "b"].' in text + # ...while the internals stay gated behind the debug flags. + assert "internal detail that must stay hidden" not in text + assert "Traceback" not in text diff --git a/libs/tests/core/test_invalid_input_guidance.py b/libs/tests/core/test_invalid_input_guidance.py new file mode 100644 index 000000000..a52637a7e --- /dev/null +++ b/libs/tests/core/test_invalid_input_guidance.py @@ -0,0 +1,236 @@ +"""Tests for the actionable guidance attached to invalid-tool-input errors. + +An input validation failure used to state only what was wrong ("age: Input +should be a valid integer") without saying what the tool actually expects, so +the caller had to go re-read the schema to self-correct. These tests pin the +appended "Expected:" block, and — just as importantly — pin that it is built +from the tool's *declared schema* and never from the rejected values, which may +contain secrets or PII. +""" + +from typing import Annotated, Literal, Optional + +import pytest +from arcade_core.catalog import ToolCatalog +from arcade_core.executor import ToolExecutor, _expected_shape_guidance +from arcade_core.schema import ToolContext +from arcade_tdk import tool + +catalog = ToolCatalog() + + +@tool +def weather_tool( + city: Annotated[str, "The city to look up"], + units: Annotated[Optional[Literal["c", "f"]], "Temperature units"] = "c", + days: Annotated[Optional[int], "Number of forecast days"] = 1, +) -> Annotated[str, "forecast"]: + """Look up a forecast.""" + return f"{city} {units} {days}" + + +@tool +def tags_tool( + tags: Annotated[list[str], "A list of tags"], +) -> Annotated[str, "output"]: + """Tool taking a list.""" + return ",".join(tags) + + +catalog.add_tool(weather_tool, "GuidanceToolkit") +catalog.add_tool(tags_tool, "GuidanceToolkit") + + +async def _run(func, **kwargs): + definition = catalog.find_tool_by_func(func) + materialized = catalog.get_tool(definition.get_fully_qualified_name()) + return await ToolExecutor.run( + func=func, + definition=definition, + input_model=materialized.input_model, + output_model=materialized.output_model, + context=ToolContext(), + **kwargs, + ) + + +class TestExpectedShapeGuidance: + @pytest.mark.asyncio + async def test_missing_required_field_reports_expected_shape(self): + output = await _run(weather_tool) # omit required 'city' + + assert output.error is not None + msg = output.error.message + # The existing "what went wrong" half is preserved. + assert "Invalid input:" in msg + assert "city" in msg + # The new "what is expected" half. + assert "Expected:" in msg + assert "string" in msg + assert "required" in msg + + @pytest.mark.asyncio + async def test_guidance_includes_parameter_description(self): + output = await _run(weather_tool) + + assert output.error is not None + assert "The city to look up" in output.error.message + + @pytest.mark.asyncio + async def test_guidance_lists_allowed_values_for_enums(self): + output = await _run(weather_tool, city="Paris", units="kelvin") + + assert output.error is not None + msg = output.error.message + assert "units" in msg + # The closed set is the single most actionable fact for an enum. + assert "c" in msg and "f" in msg + + @pytest.mark.asyncio + async def test_guidance_describes_only_rejected_fields(self): + """The client already has the full schema from tools/list; repeating it + on every failure is noise. Only the fields that were actually rejected + get described.""" + output = await _run(weather_tool, city="Paris", days="not-an-int") + + assert output.error is not None + msg = output.error.message + assert "days" in msg + # 'city' validated fine, so it must not appear in the Expected block. + expected_block = msg.split("Expected:", 1)[1] + assert "city" not in expected_block + + @pytest.mark.asyncio + async def test_guidance_renders_array_element_type(self): + output = await _run(tags_tool, tags="not-a-list") + + assert output.error is not None + msg = output.error.message + assert "tags" in msg + assert "array" in msg + + @pytest.mark.asyncio + async def test_guidance_tells_the_caller_what_to_do_next(self): + output = await _run(weather_tool) + + assert output.error is not None + assert "call the tool again" in output.error.message.lower() + + +class TestGuidanceNeverLeaksInputValues: + """The guidance is built from the declared schema, so adding it must not + reintroduce the value-echo that the executor deliberately avoids.""" + + @pytest.mark.asyncio + async def test_rejected_values_absent_from_message_and_developer_message(self): + sentinel = "SENTINEL_TOKEN_DO_NOT_LEAK_99" + output = await _run(weather_tool, city=sentinel, days=sentinel) + + assert output.error is not None + assert sentinel not in output.error.message + assert output.error.developer_message is not None + assert sentinel not in output.error.developer_message + + @pytest.mark.asyncio + async def test_enum_guidance_does_not_echo_the_rejected_value(self): + sentinel = "SENTINEL_UNIT_VALUE_77" + output = await _run(weather_tool, city="Paris", units=sentinel) + + assert output.error is not None + assert sentinel not in output.error.message + + +class TestBackwardCompatibility: + @pytest.mark.asyncio + async def test_message_still_starts_with_invalid_input_summary(self): + """Callers (and existing tests) match on the leading + ``Invalid input: : `` summary; guidance is appended + after it, never spliced into it.""" + output = await _run(tags_tool, tags="not-a-list") + + assert output.error is not None + head = output.error.message.split("Expected:", 1)[0] + assert "Invalid input: tags:" in head + + @pytest.mark.asyncio + async def test_developer_message_shape_unchanged(self): + output = await _run(tags_tool, tags="not-a-list") + + assert output.error is not None + assert output.error.developer_message is not None + assert "Pydantic validation failed:" in output.error.developer_message + + @pytest.mark.asyncio + async def test_no_definition_degrades_gracefully(self): + """``_serialize_input`` must still work without a definition (it is an + optional enrichment, not a new requirement).""" + definition = catalog.find_tool_by_func(tags_tool) + materialized = catalog.get_tool(definition.get_fully_qualified_name()) + + from arcade_core.errors import ToolInputError + + with pytest.raises(ToolInputError) as exc_info: + await ToolExecutor._serialize_input(materialized.input_model, tags="not-a-list") + + assert "Invalid input: tags:" in str(exc_info.value) + + +class TestGuidanceHelperDegradesQuietly: + """The helper is best-effort: when it cannot describe anything it returns an + empty string so callers can append unconditionally, rather than raising and + turning a validation error into a crash.""" + + def test_no_definition_returns_empty(self): + assert _expected_shape_guidance(None, ["city"]) == "" + + def test_no_rejected_fields_returns_empty(self): + definition = catalog.find_tool_by_func(weather_tool) + assert _expected_shape_guidance(definition, []) == "" + + def test_definition_without_parameters_returns_empty(self): + """A definition whose ``input`` does not expose ``parameters`` (a + partially-built or foreign definition) must not raise.""" + + class _NoParams: + input = object() + + assert _expected_shape_guidance(_NoParams(), ["city"]) == "" + + def test_unknown_rejected_field_is_skipped(self): + """An extra argument that maps to no declared parameter has no shape to + describe; the declared ones are still reported.""" + definition = catalog.find_tool_by_func(weather_tool) + guidance = _expected_shape_guidance(definition, ["not_a_parameter", "city"]) + + assert "city" in guidance + assert "not_a_parameter" not in guidance + + def test_only_unknown_fields_returns_empty(self): + definition = catalog.find_tool_by_func(weather_tool) + assert _expected_shape_guidance(definition, ["nope", "also_nope"]) == "" + + +class TestModelLevelValidationErrors: + @pytest.mark.asyncio + async def test_error_without_a_field_location_is_handled(self): + """Model-level validators report an empty ``loc``. That maps onto no + single parameter, so it must be skipped when collecting rejected field + names instead of indexing off the end.""" + from pydantic import BaseModel, model_validator + + from arcade_core.errors import ToolInputError + + class _WholeModelRejects(BaseModel): + value: str = "ok" + + @model_validator(mode="after") + def _always_fails(self): + raise ValueError("the whole model is unacceptable") + + with pytest.raises(ToolInputError) as exc_info: + await ToolExecutor._serialize_input(_WholeModelRejects, None, value="x") + + message = str(exc_info.value) + assert "Invalid input:" in message + # No parameter could be named, so no Expected block is appended. + assert "Expected:" not in message