fix(arcade-mcp-server): make invalid tool input errors actionable and stop leaking internals

[HBX814]

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 with str(error) — which renders the whole ToolCallError pydantic model, so developer_message and stacktrace went out as field=value pairs.

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_input carries an explicit comment that rejected values must stay out of the surfaced fields because they may hold secrets or PII, and _debug_exposure exists 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:

Invalid input: units: Input should be 'c' or 'f'; days: Input should be a valid integer, unable to parse string as an integer

After:

Invalid input: units: Input should be 'c' or 'f'; days: Input should be a valid integer, unable to parse string as an integer

Expected:
  - units (string, optional): Temperature units [allowed values: c, f]
  - days (integer, optional): Number of forecast days

Fix these arguments and call the tool again.

Refs: #703 — deliberately not Resolves: this covers the one error category of that issue
that 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_input is 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 new definition parameter is positional-only (/). **kwargs holds 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 raising "got multiple values for argument". This also closes the same latent collision that already existed for input_model.

definition is 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_content and routes internals through augment_error_message_for_debug rather 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:

• Most of Audit error messages for actionable fix instructions #703 is already done. Missing secrets, reserved secrets, authorization failures, transport restrictions, and the generic tool-execution failure already emit the ✗ … 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.
• The startup missing-secret warning (_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.
• Transport errors ("port in use"), item 4 of Audit error messages for actionable fix instructions #703, are unhandled — uvicorn.run() surfaces the raw OSError. That needs a different area of the code and reads better as its own PR.
• The legacy branch also skips _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.

• Confirmed the tests fail on the current code first. The pre-fix run reproduced the leak exactly — the client message was 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.
• Verified the leak directly at the source: for a bad argument, the sentinel value is absent from message and developer_message but present in stacktrace, and therefore present in str(error) — which is what the legacy branch sent.
• libs/tests/arcade_mcp_server/test_invalid_input_legacy_protocol.py (8): rejected value not echoed; no Traceback; no kind= / 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 without parameters, an unrecognized field, and a model-level error with no field location).
• The legacy test resolves the tool name from the catalog rather than hardcoding it. An early version hardcoded needs_a_list, but Arcade derives NeedsAList, so the server answered "Unknown tool" and three absence-based assertions passed vacuously. _legacy_error_text now asserts the call actually reached input validation, so that failure mode cannot recur.
• Full libs/tests/ run: 3225 passed, 532 skipped (3200 before, +25 new). No regressions — notably test_executor.py's startswith("… Invalid input: inp:") assertions and test_input_validation_error_does_not_leak_input_values still pass.
• The 2 failures in arcade_mcp_server/integration/test_end_to_end.py (test_stdio_e2e, test_http_e2e) are pre-existing on main — verified by stashing this change and re-running — a local server-spawn/port issue on Windows.
• pre-commit run -a (what CI's quality job runs) fully clean, including the check-debug-leak-flags guard; ruff check and ruff format --check clean; mypy clean on arcade-core (33 files) and arcade-mcp-server (51 files).
• Versions bumped once on this branch: arcade-core 4.11.0 → 4.12.0 (additive behavior change), arcade-mcp-server 1.26.0 → 1.26.1 (defect fix). test_dependency_alignment.py passes; 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 quality and Debug leak flag guard, both from one mistake of mine: the legacy test hardcoded the debug-flag activation acknowledgement string, which scripts/check_debug_leak_flags_off.py forbids outside a small allowlist. It passed locally only because that guard scans git ls-files and 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_MESSAGE from _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 by pre-commit run -a.

Codecov also flagged 4 uncovered patch lines. All four were defensive branches in the new helper plus the additional_prompt_content branch; they are now covered by the added tests. The lines still uncovered in executor.py (111, 115, 140-141) and server.py (1627) are pre-existing, outside this diff.

Risk note

Change 1 touches how tool errors are surfaced, which is reachable from context.get_secret()-adjacent flows, so worth stating precisely:

• 2025-06-18 clients, invalid input: previously received the model repr including the rejected value; now receive the curated message. Any consumer that was parsing field=value pairs out of that string would break — but that string was never a documented contract, and the sibling protocol version never emitted it.
• 2025-11-25 clients: unchanged.
• Debug flags: unchanged. Internals still reach the client when ARCADE_DEBUG_EXPOSE_* is set to the ack value, on both paths.
• Change 2 only appends to a message. It is skipped entirely when no ToolDefinition is available, and no rejected value can enter it by construction.
• MCP stdio channel is untouched: no new stdout/stderr writes, per the repo's stdio-channel rule.

[codecov]

Codecov Report

❌ Patch coverage is 91.48936% with 4 lines in your changes missing coverage. Please review.

Files with missing lines	Patch %	Lines
libs/arcade-core/arcade_core/executor.py	93.02%	3 Missing ⚠️
libs/arcade-mcp-server/arcade_mcp_server/server.py	75.00%	1 Missing ⚠️

Files with missing lines	Coverage Δ
libs/arcade-mcp-server/arcade_mcp_server/server.py	85.87% <75.00%> (-0.05%)	⬇️
libs/arcade-core/arcade_core/executor.py	95.34% <93.02%> (-2.43%)	⬇️

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.