From 87aa6266dfce1c3f7da84d0e7547eab56ecfabf1 Mon Sep 17 00:00:00 2001 From: HBX814 <170509393+HBX814@users.noreply.github.com> Date: Tue, 25 Aug 2026 01:47:42 +0530 Subject: [PATCH 1/3] fix(arcade-mcp-server): stop dumping error internals on legacy input errors MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Input validation failures are version-gated: 2025-11-25 clients receive a CallToolResult, 2025-06-18 clients a JSON-RPC -32602. The legacy branch built that response with `str(error)`, which renders the entire ToolCallError pydantic model, so `developer_message` and `stacktrace` were shipped to the client as `field=value` pairs. For an input-validation error the stacktrace is a Pydantic traceback, and Pydantic embeds `input_value=` in it — so the rejected argument was echoed back verbatim. `_serialize_input` deliberately keeps rejected values out of `message` and `developer_message` because they may hold secrets or PII, and `_debug_exposure` exists so stacktraces reach a client only behind an explicit opt-in env flag. This branch bypassed both. Use the curated `error.message`, keep `additional_prompt_content` (caller-facing guidance, not an internal), and route internals through `augment_error_message_for_debug`, matching the 2025-11-25 branch, so the debug flags remain the only way to expose internals. The test reads the flag names and activation acknowledgement from `_debug_exposure` instead of restating them, so the ack string stays confined to the allowlist in scripts/check_debug_leak_flags_off.py. Refs: #703 --- .../arcade_mcp_server/server.py | 20 +- libs/arcade-mcp-server/pyproject.toml | 2 +- .../test_invalid_input_legacy_protocol.py | 212 ++++++++++++++++++ 3 files changed, 232 insertions(+), 2 deletions(-) create mode 100644 libs/tests/arcade_mcp_server/test_invalid_input_legacy_protocol.py diff --git a/libs/arcade-mcp-server/arcade_mcp_server/server.py b/libs/arcade-mcp-server/arcade_mcp_server/server.py index 81f9d0e69..2aeeca36a 100644 --- a/libs/arcade-mcp-server/arcade_mcp_server/server.py +++ b/libs/arcade-mcp-server/arcade_mcp_server/server.py @@ -1600,9 +1600,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 23aa82ac2..2d692ee3c 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.32.0" +version = "1.32.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 From f78b8cd8443ebf77e47754959b479790a56598a2 Mon Sep 17 00:00:00 2001 From: HBX814 <170509393+HBX814@users.noreply.github.com> Date: Tue, 25 Aug 2026 01:47:43 +0530 Subject: [PATCH 2/3] feat(arcade-core): tell callers the expected shape on invalid tool input MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit An input validation failure reported only what was wrong ("age: Input should be a valid integer") and left the caller to go re-read the schema to work out what the tool actually wanted. Append an "Expected:" block describing the declared shape of the parameters that were rejected — type, required/optional, description, and the allowed values for closed sets — followed by an explicit next step. The block is built strictly from the tool's own ToolDefinition, never from the submitted values, preserving the existing guarantee that rejected input never reaches the surfaced message. Only rejected parameters are described: the caller already has the full schema from tools/list, so repeating it on every failure would bury the actionable part. The leading "Invalid input: : " summary is unchanged, so existing callers matching on it keep working. The helper degrades to an empty string whenever it cannot describe anything — no definition, an unrecognized field, or a model-level error with no field location — so enrichment can never turn a validation error into a crash. Refs: #703 --- libs/arcade-core/arcade_core/executor.py | 91 ++++++- libs/arcade-core/pyproject.toml | 2 +- .../tests/core/test_invalid_input_guidance.py | 236 ++++++++++++++++++ 3 files changed, 325 insertions(+), 4 deletions(-) create mode 100644 libs/tests/core/test_invalid_input_guidance.py 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 e659b25ec..d2a865402 100644 --- a/libs/arcade-core/pyproject.toml +++ b/libs/arcade-core/pyproject.toml @@ -1,6 +1,6 @@ [project] name = "arcade-core" -version = "4.21.0" +version = "4.22.0" description = "Arcade Core - Core library for Arcade platform" readme = "README.md" license = { text = "MIT" } 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 From 422e82999d7fd700626b4fa16a7387d0ab804710 Mon Sep 17 00:00:00 2001 From: HBX814 <170509393+HBX814@users.noreply.github.com> Date: Wed, 16 Sep 2026 17:18:49 +0530 Subject: [PATCH 3/3] fix(arcade-core): describe renamed parameters in expected-shape guidance MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit A two-string ``Annotated[T, "wire_name", "description"]`` renames a parameter for the wire: ``extract_field_info`` sets ``InputParameter.name`` to the first string, while ``create_func_models`` keys the Pydantic model on the Python parameter name. Validation errors report the Pydantic name, so looking the rejected field up only against ``InputParameter.name`` missed every renamed parameter and dropped the "Expected:" block entirely — including for the most common case, a missing required argument. Resolve both names. ``create_input_definition`` and ``create_func_models`` walk the same ``inspect.signature`` in the same order and skip ``ToolContext`` the same way, so the declared parameters line up positionally with the model fields; Python names are added with ``setdefault`` so a wire name is never shadowed by another parameter's Python name. Calling with the wire name rejects one parameter under both names at once ("py_param: Field required; wire_name: Extra inputs are not permitted"), so parameters are de-duplicated and described once. The guidance still shows the wire name, which is what the caller knows the parameter as from tools/list. Reported by Cursor Bugbot on #913. --- libs/arcade-core/arcade_core/executor.py | 32 ++++++++- .../tests/core/test_invalid_input_guidance.py | 72 ++++++++++++++++++- 2 files changed, 100 insertions(+), 4 deletions(-) diff --git a/libs/arcade-core/arcade_core/executor.py b/libs/arcade-core/arcade_core/executor.py index d271a4fe6..fb509a13d 100644 --- a/libs/arcade-core/arcade_core/executor.py +++ b/libs/arcade-core/arcade_core/executor.py @@ -31,6 +31,7 @@ def _render_declared_type(value_schema: Any) -> str: def _expected_shape_guidance( definition: ToolDefinition | None, rejected_fields: list[str], + input_model: type[BaseModel] | None = None, ) -> str: """Describe the declared shape of the parameters that were rejected. @@ -40,6 +41,15 @@ def _expected_shape_guidance( caller already received the full schema from ``tools/list``, so echoing all of it on every failure is noise that buries the actionable part. + ``rejected_fields`` holds Pydantic field names, which are the *Python* + parameter names. A two-string ``Annotated[T, "wire_name", "description"]`` + renames the parameter for the wire, so ``InputParameter.name`` is not always + the Pydantic field name and a name-only lookup would silently miss every + renamed parameter. ``input_model`` supplies the Pydantic side so both names + resolve. ``create_input_definition`` and ``create_func_models`` walk the same + ``inspect.signature`` in the same order and skip ``ToolContext`` the same + way, so the two sequences line up positionally. + Returns an empty string when there is nothing useful to add, so callers can append unconditionally. """ @@ -47,17 +57,35 @@ def _expected_shape_guidance( return "" try: - parameters = {param.name: param for param in definition.input.parameters} + declared = list(definition.input.parameters) except AttributeError: return "" + # Wire names first; Python field names only fill gaps, so a parameter whose + # wire name collides with another's Python name is never shadowed. + parameters = {param.name: param for param in declared} + if input_model is not None: + try: + field_names = list(input_model.model_fields) + except AttributeError: + field_names = [] + for field_name, declared_param in zip(field_names, declared): + parameters.setdefault(field_name, declared_param) + lines: list[str] = [] + described: set[int] = set() 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 + if id(param) in described: + # One renamed parameter can be rejected under both of its names at + # once ("py_param: Field required; wire_name: Extra inputs are not + # permitted"). Describe it once. + continue + described.add(id(param)) qualifier = "required" if param.required else "optional" line = f" - {param.name} ({_render_declared_type(param.value_schema)}, {qualifier})" if param.description: @@ -196,7 +224,7 @@ async def _serialize_input( rejected_fields.append(field) message = f"Invalid input: {summary}" - guidance = _expected_shape_guidance(definition, rejected_fields) + guidance = _expected_shape_guidance(definition, rejected_fields, input_model) if guidance: message = f"{message}\n\n{guidance}" diff --git a/libs/tests/core/test_invalid_input_guidance.py b/libs/tests/core/test_invalid_input_guidance.py index a52637a7e..09f55ae18 100644 --- a/libs/tests/core/test_invalid_input_guidance.py +++ b/libs/tests/core/test_invalid_input_guidance.py @@ -37,8 +37,17 @@ def tags_tool( return ",".join(tags) +@tool +def renamed_param_tool( + py_param: Annotated[int, "wire_name", "A number the caller passes"], +) -> Annotated[str, "output"]: + """Tool whose parameter is renamed for the wire.""" + return str(py_param) + + catalog.add_tool(weather_tool, "GuidanceToolkit") catalog.add_tool(tags_tool, "GuidanceToolkit") +catalog.add_tool(renamed_param_tool, "GuidanceToolkit") async def _run(func, **kwargs): @@ -216,9 +225,8 @@ 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 + from pydantic import BaseModel, model_validator class _WholeModelRejects(BaseModel): value: str = "ok" @@ -234,3 +242,63 @@ def _always_fails(self): assert "Invalid input:" in message # No parameter could be named, so no Expected block is appended. assert "Expected:" not in message + + +class TestRenamedParameters: + """A two-string ``Annotated[T, "wire_name", "description"]`` renames the + parameter for the wire, so ``InputParameter.name`` and the Pydantic field + name diverge. Validation errors report the Pydantic (Python) name, so a + lookup keyed only on the declared name silently skipped these parameters + and dropped the guidance entirely.""" + + @pytest.mark.asyncio + async def test_missing_renamed_required_field_still_gets_guidance(self): + output = await _run(renamed_param_tool) + + assert output.error is not None + msg = output.error.message + assert "Expected:" in msg + # The caller knows this parameter by its wire name, so that is what the + # guidance must show. + assert "wire_name" in msg + assert "A number the caller passes" in msg + + @pytest.mark.asyncio + async def test_wrong_type_on_renamed_field_still_gets_guidance(self): + output = await _run(renamed_param_tool, py_param="not-an-int") + + assert output.error is not None + msg = output.error.message + assert "Expected:" in msg + assert "wire_name" in msg + assert "integer" in msg + + @pytest.mark.asyncio + async def test_parameter_rejected_under_both_names_is_described_once(self): + """Calling with the wire name yields two errors for one parameter + ("py_param: Field required" and "wire_name: Extra inputs are not + permitted"). It must still be described a single time.""" + output = await _run(renamed_param_tool, wire_name="not-an-int") + + assert output.error is not None + expected_block = output.error.message.split("Expected:", 1)[1] + assert expected_block.count("wire_name (") == 1 + + def test_helper_resolves_both_names(self): + definition = catalog.find_tool_by_func(renamed_param_tool) + materialized = catalog.get_tool(definition.get_fully_qualified_name()) + + by_python_name = _expected_shape_guidance( + definition, ["py_param"], materialized.input_model + ) + by_wire_name = _expected_shape_guidance(definition, ["wire_name"], materialized.input_model) + + assert "wire_name" in by_python_name + assert "wire_name" in by_wire_name + + def test_helper_without_input_model_still_resolves_declared_names(self): + """``input_model`` is optional enrichment; the declared-name lookup must + keep working when it is absent.""" + definition = catalog.find_tool_by_func(renamed_param_tool) + + assert "wire_name" in _expected_shape_guidance(definition, ["wire_name"])