[Server][Capability] Cast variadic tool parameters element-by-element - #465
Conversation
SchemaGenerator::buildVariadicParameterSchema() already advertises a variadic parameter (e.g. `int ...$scores`) as a JSON "array" schema, so a spec-conformant client sends an array for it. But ReferenceHandler::prepareArguments() never special-cased ReflectionParameter::isVariadic(): it fell through to the regular single-value branch and passed the whole array to castArgumentType(), which tried to cast it as one scalar and failed - e.g. "Cannot cast value to integer" for `int ...$scores` given [1, 2, 3]. Any tool handler declaring a variadic parameter was therefore unreachable through the advertised schema, confirmed by direct reproduction against ReferenceHandler::handle() before this change. Casts each array element individually via the existing castArgumentType() (unchanged - it already inspects the variadic parameter's element type, not "array", per PHP reflection semantics) and appends to the result in order; variadic is always the last parameter, so this preserves correct positional ordering. A non-array value for a variadic argument now fails clearly with an invalid-params error instead of the confusing cast message.
chr-hertel
left a comment
There was a problem hiding this comment.
Thanks for tracking this down, @Faneraiy14 — the diagnosis is right, including the bit that trips most people up: getType() on int ...$scores returns int, not array, so castArgumentType() genuinely needs no changes. The ordering argument holds as well.
Four notes inline. The one on the non-array rejection is a real regression, the rest are polish.
One more thing that can't be anchored to a line: please prefix the title with the component to match the repo convention — [Capability] Cast variadic tool parameters element-by-element.
Nothing in the repo exercises variadics end-to-end right now, which is exactly why the prompt case below stays invisible. I'll follow up with an example and docs once this lands.
| // SchemaGenerator advertises variadic parameters as a JSON "array" | ||
| // schema (see buildVariadicParameterSchema()), so the incoming value | ||
| // here is an array whose elements each need casting to the variadic's | ||
| // element type. Falling through to castArgumentType() below would try | ||
| // to cast the whole array as a single scalar and fail (e.g. "Cannot | ||
| // cast value to integer" for `int ...$extra`). Variadic is always the | ||
| // last parameter, so appending here preserves correct final ordering. |
There was a problem hiding this comment.
Seven lines of rationale is heavier than the rest of this file, and the PR description already carries it. Two lines would do.
| // cast value to integer" for `int ...$extra`). Variadic is always the | ||
| // last parameter, so appending here preserves correct final ordering. | ||
| $values = $arguments[$paramName] ?? []; | ||
| if (!\is_array($values)) { |
There was a problem hiding this comment.
This regresses behaviour that works on main today: ['topics' => 'php'] for string ...$topics currently yields ['php'].
It matters for prompts. prompts/get arguments are Record<string, string> by protocol — the Inspector sends userIds=["101","102"] through as a literal string — so a variadic prompt parameter fails here where it works now.
Wrapping a non-array in a one-element list instead of throwing keeps both cases working.
| if (!\is_array($values)) { | ||
| throw RegistryException::invalidParams(\sprintf('Parameter `%s` must be an array of values.', $paramName)); | ||
| } | ||
| foreach (array_values($values) as $value) { |
There was a problem hiding this comment.
array_values() silently accepts a JSON object and flattens it into positional args, even though the advertised schema is array. array_is_list() plus a reject would be tighter.
| try { | ||
| $finalArgs[] = $this->castArgumentType($value, $parameter); | ||
| } catch (InvalidArgumentException $e) { | ||
| throw RegistryException::invalidParams($e->getMessage(), $e); |
There was a problem hiding this comment.
Drops which element failed. Cannot cast value to integer on a 20-item array is hard to act on — worth including the parameter name and the index.
… errors - Non-array variadic argument is now wrapped as a single-element list instead of being rejected: MCP prompt arguments are always Record<string,string> per protocol, so a variadic prompt parameter legitimately receives a plain string, not an array. Rejecting it regressed behaviour that worked on main. - A JSON object (non-list array) is now explicitly rejected instead of being silently flattened into positional args by array_values(). - Cast-failure errors now include the parameter name and element index, so a failure inside a large array is actually actionable. - Trimmed the inline rationale comment; the PR description already covers it. Tests updated: replaced the "throws on non-array" test (no longer correct) with tests for the wrap-as-single-element, reject-object, and indexed-error-message behaviours.
|
good catch on the prompt-argument case, missed that entirely. fixed all 4:
tests updated to match - replaced the old "throws on non-array" test since that's no longer the behavior, added tests for the wrap case, the object-rejection case, and the indexed error message. 1518 unit tests + phpstan level 6 still green. renamed the title too. |
chr-hertel
left a comment
There was a problem hiding this comment.
Thanks @Faneraiy14 - just added a small additional test, but will merge in a second 👍
Summary
SchemaGenerator::buildVariadicParameterSchema()already advertises a variadic parameter (e.g.int ...$scores) as a JSON"array"schema, so a spec-conformant client sends an array for it. ButReferenceHandler::prepareArguments()never special-casedReflectionParameter::isVariadic()— it fell through to the regular single-value branch and passed the whole array tocastArgumentType(), which tried to cast it as one scalar and failed (e.g."Cannot cast value to integer"forint ...$scoresgiven[1, 2, 3]).Any tool handler declaring a variadic parameter was therefore unreachable through the advertised schema — confirmed by direct reproduction against
ReferenceHandler::handle()before this change:This resolves the
// TODO: Handle variadic parameters.left inprepareArguments().Fix
Casts each array element individually via the existing
castArgumentType()(unchanged — it already inspects the variadic parameter's element type, notarray, per PHP reflection semantics) and appends to the result in order. Variadic is always the last parameter, so this preserves correct positional ordering. A non-array value for a variadic argument now fails clearly with an invalid-params error instead of the confusing cast message.Test plan
vendor/bin/phpunit --testsuite=unit— 1516 tests, 3923 assertions, all passingvendor/bin/phpstan analyse(full repo, level 6 perphpstan.dist.neon) — no errors