Conversation
Codecov Report❌ Patch coverage is
🚀 New features to boost your workflow:
|
71b4384 to
a609815
Compare
|
This pull request has been automatically marked as stale because it has had no activity for 14 days. It will be closed in 14 days if no further activity occurs. If this is still relevant, please leave a comment or remove the stale label. |
…errors 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: ArcadeAI#703
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: <field>: <reason>" 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: ArcadeAI#703
a609815 to
99528a7
Compare
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes using high effort and found 1 potential issue.
Reviewed by Cursor Bugbot for commit 99528a7. Configure here.
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 ArcadeAI#913.
|
Confirmed and fixed in 214c6e2 — thanks, this was a real miss. Verified the mechanism rather than taking it on faith. Worth noting it hit the most common case — a missing required argument — not just an edge case. The fix resolves both names. One thing that surfaced while fixing it: calling with the wire name rejects the same parameter under both names at once ( Five tests added. Three of them fail against the previous commit (the two behavioural ones plus the helper), so the regression is actually pinned rather than passing vacuously. Separately, the probe showed that calling a renamed parameter by its advertised wire name fails validation outright ( |
1 participant
Summary
Two changes to the invalid-tool-input path, found while auditing #703.
1. The legacy branch shipped error internals to the client. Input validation failures are version-gated: 2025-11-25 clients get a
CallToolResult, 2025-06-18 clients get a JSON-RPC-32602. The legacy branch built that response withstr(error)— which renders the wholeToolCallErrorpydantic model, sodeveloper_messageandstacktracewent out asfield=valuepairs.That matters more than cosmetics. 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_inputcarries an explicit comment that rejected values must stay out of the surfaced fields because they may hold secrets or PII, and_debug_exposureexists so stacktraces reach a client only behind an opt-in env flag. This branch bypassed both.2. Invalid-input errors now say what the tool expects — the "Invalid tool input — show expected shape" bullet from #703.
Before:
After:
Refs: #703 — deliberately not
Resolves: this covers the one error category of that issuethat had not already been converted. See Scope for what remains.
Design decisions
The guidance is built from the
ToolDefinition, never from the submitted values. #703 phrases this category as "show expected shape vs received", but echoing received values is exactly what_serialize_inputis written to avoid — a naive reading of the issue would reintroduce the leak that change 1 above removes. So only the declared schema is rendered. Two tests pin this by passing a sentinel value and asserting it appears nowhere.Only the rejected parameters are described. The caller already received the full schema from
tools/list; repeating all of it on every failure buries the actionable part and grows unbounded with the tool's arity. The block is bounded by the number of errors instead.The leading
Invalid input: <field>: <reason>summary is unchanged, and guidance is appended after it. Existing tests (and any caller) match on that prefix, so it stays byte-identical; nothing is spliced into it._serialize_input's newdefinitionparameter is positional-only (/).**kwargsholds caller-supplied tool arguments, and a tool is free to declare a parameter nameddefinition— orinput_model. Positional-only placement keeps such an argument inkwargsinstead of raising "got multiple values for argument". This also closes the same latent collision that already existed forinput_model.definitionis optional and the helper returns""when it cannot describe anything. Validation still works without a definition, so this is enrichment rather than a new requirement, and callers can append unconditionally.Change 1 keeps
additional_prompt_contentand routes internals throughaugment_error_message_for_debugrather than dropping them, so the documented debug escape hatch still works on the legacy path — it just becomes the only way to expose internals there. Two tests assert the flags still surface[DEBUG] stacktrace:/[DEBUG] developer_message:.Scope
In scope: the "invalid tool input" category of #703, plus the internals leak found on the same path.
Not in scope:
✗ … To fix:shape the issue asked for. I only touched the category that hadn't been converted. Worth a maintainer confirming whether Audit error messages for actionable fix instructions #703 should stay open for the rest._check_and_warn_missing_secrets) still says only "declares secret(s) 'X' which is/are not set. It will return an error if called." — no fix instructions, while the runtime error for the same condition has them. Small consistency gap, deliberately left out to keep this diff reviewable.uvicorn.run()surfaces the rawOSError. That needs a different area of the code and reads better as its own PR._log_tool_call_error/_record_tool_error_span_attributes, which the 2025-11-25 branch calls. That looks like an oversight, but changing logging/telemetry behavior is outside a message fix, so I left it alone rather than widen the blast radius.Test plan
Two new files, 25 tests, written before the implementation, per the repo's TDD requirement.
message="[TOOL_RUNTIME_BAD_INPUT_VALUE] ToolInputError … Invalid input: tags…=str]\n For further information visit https://errors.pydantic.dev/2.13/v/list_type\n' status_code=400 extra=None, i.e. the full model repr with the Pydantic error inside.messageanddeveloper_messagebut present instacktrace, and therefore present instr(error)— which is what the legacy branch sent.libs/tests/arcade_mcp_server/test_invalid_input_legacy_protocol.py(8): rejected value not echoed; noTraceback; nokind=/developer_message=/can_retry=/status_code=model-repr fields; message still actionable; both debug flags still work; modern path unchanged.libs/tests/core/test_invalid_input_guidance.py(17): expected shape, description, enum allowed-values, array element type, only-rejected-fields, next-step line, two no-leak tests, three backward-compatibility tests, and five covering the helper's quiet-degradation paths (no definition, no rejected fields, a definition withoutparameters, an unrecognized field, and a model-level error with no field location).needs_a_list, but Arcade derivesNeedsAList, so the server answered "Unknown tool" and three absence-based assertions passed vacuously._legacy_error_textnow asserts the call actually reached input validation, so that failure mode cannot recur.libs/tests/run: 3425 passed, 532 skipped (3400 onmainbefore, +25 new). No regressions — notablytest_executor.py'sstartswith("… Invalid input: inp:")assertions andtest_input_validation_error_does_not_leak_input_valuesstill pass.arcade_mcp_server/integration/test_end_to_end.py(test_stdio_e2e,test_http_e2e) are pre-existing onmain— verified by stashing this change and re-running — a local server-spawn/port issue on Windows.pre-commit run -a(what CI'squalityjob runs) fully clean, including thecheck-debug-leak-flagsguard;ruff checkandruff format --checkclean;mypyclean onarcade-core(33 files) andarcade-mcp-server(51 files).arcade-core4.18.0 → 4.19.0 (additive behavior change),arcade-mcp-server1.30.1 → 1.30.2 (defect fix).test_dependency_alignment.pypasses; the root constraints (arcade-core>=4.9.0,arcade-mcp-server>=1.23.0) already admit both, and no dependency floor needed raising since neither change is breaking.Note on the first CI run
The first push failed
qualityandDebug leak flag guard, both from one mistake of mine: the legacy test hardcoded the debug-flag activation acknowledgement string, whichscripts/check_debug_leak_flags_off.pyforbids outside a small allowlist. It passed locally only because that guard scansgit ls-filesand the file was still untracked when I ran the hooks.Rather than widen the allowlist, the test now reads
_DEBUG_LEAK_MAGIC,_ENV_EXPOSE_STACKTRACE, and_ENV_EXPOSE_DEVELOPER_MESSAGEfrom_debug_exposure, so the ack string stays confined to the files already permitted to contain it — and the constants have a single source of truth. Verified by running the guard script directly with the file staged, and bypre-commit run -a.Codecov also flagged 4 uncovered patch lines. All four were defensive branches in the new helper plus the
additional_prompt_contentbranch; they are now covered by the added tests. The lines still uncovered inexecutor.py(111, 115, 140-141) andserver.py(1627) are pre-existing, outside this diff.Rebased onto current
main(Sep 16)The branch had gone stale while waiting on workflow approval, so it is rebased onto
main(7246e9d6). Two conflicts, both version bumps only —mainhad movedarcade-core4.11.0 → 4.18.0 andarcade-mcp-server1.26.0 → 1.30.1, leaving the original bumps belowmain. Re-bumped from the current numbers; the code changes themselves merged without conflict.Re-checked that the fix is still needed rather than assuming:
str(error)is still on the legacy bad-input branch onmaintoday, and the merged result places the replacement correctly.Risk note
Change 1 touches how tool errors are surfaced, which is reachable from
context.get_secret()-adjacent flows, so worth stating precisely:field=valuepairs out of that string would break — but that string was never a documented contract, and the sibling protocol version never emitted it.ARCADE_DEBUG_EXPOSE_*is set to the ack value, on both paths.ToolDefinitionis available, and no rejected value can enter it by construction.Note
Medium Risk
Changes how invalid-input errors are surfaced to MCP clients (security-sensitive: stops echoing secrets/PII on the legacy path) and extends user-visible error text; modern protocol behavior is unchanged aside from richer messages from core.
Overview
Fixes two problems on the invalid-tool-input path: legacy MCP clients were getting full error dumps (including rejected argument values via Pydantic stacktraces), and validation failures only said what was wrong, not what the tool expects.
In arcade-core, validation errors still start with
Invalid input: …but now append an Expected: block for only the rejected parameters, built from the tool’sToolDefinition(types, required/optional, descriptions, enums)—never from submitted values._serialize_inputtakes an optional positional-onlydefinitionso tool args nameddefinition/input_modeldon’t collide with**kwargs.In arcade-mcp-server, the 2025-06-18 JSON-RPC
-32602branch no longer usesstr(error); it sends the curatederror.message(plusadditional_prompt_content) and routes stacktrace/developer details throughaugment_error_message_for_debug, matching the newer protocol path. Package versions bump to arcade-core 4.19.0 and arcade-mcp-server 1.30.2, with new tests covering guidance, no-leak behavior, renamed wire params, and legacy debug flags.Reviewed by Cursor Bugbot for commit 214c6e2. Bugbot is set up for automated code reviews on this repo. Configure here.