Skip to content

Commit 46628fb

Browse files
Faneraiy14chr-hertel
Faneraiy14
and
chr-hertel
authored
[Capability] Cast variadic tool parameters element-by-element (#465)
* Cast variadic tool parameters element-by-element 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. * Address chr-hertel's review: wrap scalars, reject objects, index cast 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. * Pin argument order for an injectable before a variadic --------- Co-authored-by: Christopher Hertel <mail@christopher-hertel.de>
1 parent e473c3c commit 46628fb

2 files changed

Lines changed: 119 additions & 1 deletion

File tree

‎src/Capability/Registry/ReferenceHandler.php‎

Lines changed: 23 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -97,10 +97,32 @@ private function prepareArguments(\ReflectionFunctionAbstract $reflection, array
9797
$finalArgs = [];
9898

9999
foreach ($reflection->getParameters() as $parameter) {
100-
// TODO: Handle variadic parameters.
101100
$paramName = $parameter->getName();
102101
$paramPosition = $parameter->getPosition();
103102

103+
if ($parameter->isVariadic()) {
104+
// Each element is cast individually below; a non-array value is
105+
// wrapped as a single element, since prompt arguments arrive as
106+
// plain strings (Record<string,string> per the protocol) even
107+
// though tool arguments follow the advertised "array" schema.
108+
$values = $arguments[$paramName] ?? [];
109+
if (!\is_array($values)) {
110+
$values = [$values];
111+
} elseif (!array_is_list($values)) {
112+
throw RegistryException::invalidParams(\sprintf('Parameter `%s` must be a list of values, not an object.', $paramName));
113+
}
114+
foreach ($values as $index => $value) {
115+
try {
116+
$finalArgs[] = $this->castArgumentType($value, $parameter);
117+
} catch (InvalidArgumentException $e) {
118+
throw RegistryException::invalidParams(\sprintf('Parameter `%s[%d]`: %s', $paramName, $index, $e->getMessage()), $e);
119+
} catch (\Throwable $e) {
120+
throw RegistryException::internalError(\sprintf('Error processing parameter `%s[%d]`: %s', $paramName, $index, $e->getMessage()), $e);
121+
}
122+
}
123+
continue;
124+
}
125+
104126
// Check if parameter is a special injectable type
105127
$type = $parameter->getType();
106128
if ($type instanceof \ReflectionNamedType && !$type->isBuiltin()) {

‎tests/Unit/Capability/Registry/ReferenceHandlerTest.php‎

Lines changed: 96 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -14,6 +14,7 @@
1414
use Mcp\Capability\Registry\ElementReference;
1515
use Mcp\Capability\Registry\ReferenceHandler;
1616
use Mcp\Exception\InvalidArgumentException;
17+
use Mcp\Exception\RegistryException;
1718
use Mcp\Server\ClientGateway;
1819
use Mcp\Server\Handler\ResourceHandlerInterface;
1920
use Mcp\Server\Handler\ToolHandlerInterface;
@@ -167,4 +168,99 @@ public function testHandleThrowsForStringHandlerThatIsNeitherFunctionNorClass():
167168

168169
(new ReferenceHandler())->handle($reference, ['_session' => $session]);
169170
}
171+
172+
public function testHandleCastsEachElementOfAnArrayArgumentForAVariadicParameter(): void
173+
{
174+
// SchemaGenerator advertises variadic parameters as a JSON "array" schema,
175+
// so the array arrives here as a single named argument (not spread across
176+
// multiple keys) and must be cast element-by-element to the variadic's type.
177+
$closure = static fn (string $name, int ...$scores): string => \sprintf('%s:%d', $name, array_sum($scores));
178+
$reference = new ElementReference($closure);
179+
180+
$result = (new ReferenceHandler())->handle($reference, [
181+
'_session' => $this->createMock(SessionInterface::class),
182+
'name' => 'total',
183+
'scores' => ['1', '2', '3'],
184+
]);
185+
186+
$this->assertSame('total:6', $result);
187+
}
188+
189+
public function testHandleTreatsOmittedVariadicArgumentAsZeroElements(): void
190+
{
191+
$closure = static fn (string $name, int ...$scores): int => \count($scores);
192+
$reference = new ElementReference($closure);
193+
194+
$result = (new ReferenceHandler())->handle($reference, [
195+
'_session' => $this->createMock(SessionInterface::class),
196+
'name' => 'empty',
197+
]);
198+
199+
$this->assertSame(0, $result);
200+
}
201+
202+
public function testHandleWrapsANonArrayVariadicArgumentAsASingleElement(): void
203+
{
204+
// Tool arguments follow the advertised "array" schema, but MCP prompt
205+
// arguments are always Record<string,string> per the protocol - a
206+
// client sends a plain string for a variadic prompt parameter, and
207+
// that must still work rather than being rejected.
208+
$closure = static fn (string $name, string ...$topics): array => $topics;
209+
$reference = new ElementReference($closure);
210+
211+
$result = (new ReferenceHandler())->handle($reference, [
212+
'_session' => $this->createMock(SessionInterface::class),
213+
'name' => 'single',
214+
'topics' => 'php',
215+
]);
216+
217+
$this->assertSame(['php'], $result);
218+
}
219+
220+
public function testHandleThrowsRegistryExceptionWhenVariadicArgumentIsAnObjectNotAList(): void
221+
{
222+
$closure = static fn (string $name, int ...$scores): int => \count($scores);
223+
$reference = new ElementReference($closure);
224+
225+
$this->expectException(RegistryException::class);
226+
$this->expectExceptionMessage('Parameter `scores` must be a list of values, not an object.');
227+
228+
(new ReferenceHandler())->handle($reference, [
229+
'_session' => $this->createMock(SessionInterface::class),
230+
'name' => 'bad',
231+
'scores' => ['a' => 1, 'b' => 2],
232+
]);
233+
}
234+
235+
public function testHandleKeepsArgumentOrderWhenAnInjectableParameterPrecedesAVariadic(): void
236+
{
237+
// Injectable and regular parameters are assigned by position while variadic
238+
// elements are appended, so the final order only holds because a variadic is
239+
// always the last parameter.
240+
$closure = static fn (ClientGateway $gateway, string $sep = ',', string ...$parts): string => implode($sep, $parts);
241+
$reference = new ElementReference($closure);
242+
243+
$result = (new ReferenceHandler())->handle($reference, [
244+
'_session' => $this->createMock(SessionInterface::class),
245+
'_request' => new \stdClass(),
246+
'parts' => ['a', 'b'],
247+
]);
248+
249+
$this->assertSame('a,b', $result);
250+
}
251+
252+
public function testHandleIncludesParameterNameAndIndexWhenAVariadicElementFailsToCast(): void
253+
{
254+
$closure = static fn (string $name, int ...$scores): int => \count($scores);
255+
$reference = new ElementReference($closure);
256+
257+
$this->expectException(RegistryException::class);
258+
$this->expectExceptionMessage('Parameter `scores[1]`: Cannot cast value to integer. Expected integer representation.');
259+
260+
(new ReferenceHandler())->handle($reference, [
261+
'_session' => $this->createMock(SessionInterface::class),
262+
'name' => 'bad',
263+
'scores' => ['1', 'not-a-number', '3'],
264+
]);
265+
}
170266
}

0 commit comments

Comments
 (0)