Conversation
ineersa
changed the title
#[MapToolArguments] attribute to map flat payloads
wachterjohannes
left a comment
wachterjohannes
left a comment
There was a problem hiding this comment.
Solid feature: reuses buildProperties() so nullable/enum/nested/#[Schema] handling comes for free, and the tests assert the actual flattened schema and DTO round-trip, not just presence. Two nits inline, plus one CI fix.
- Fabbot's only red check is the commit title using Conventional Commits (
feat(agent): ...). This repo doesn't use that convention, worth amending the title before merge.
| $reference = $metadata->getReference(); | ||
| $method = new \ReflectionMethod($reference->getClass(), $reference->getMethod()); | ||
| $mapped = MappedToolArgument::forMethod($method); | ||
| if (null !== $mapped) { | ||
| return [ | ||
| $mapped->parameter->getName() => $this->denormalizer->denormalize($toolCall->getArguments(), $mapped->className, 'json'), | ||
| ]; | ||
| } |
There was a problem hiding this comment.
The rest of this method wraps failures as ToolException (the class docblock promises @throws ToolException). This path denormalizes straight from the DTO and lets MissingConstructorArgumentsException escape raw instead, confirmed by the PR's own test expecting that exact class. Doesn't matter through Toolbox::execute() today, both get wrapped into ToolExecutionException the same way, but it's an inconsistent contract for anyone calling the resolver directly.
There was a problem hiding this comment.
Yeah, but below
ai/src/agent/src/Toolbox/ToolCallArgumentResolver.php
Lines 115 to 117 in 258041a
Should I just add docblock to show that multiple exceptions possible or catch in both cases and produce ToolException ?
There was a problem hiding this comment.
Good catch, that gap predates this PR. For this PR, please wrap just the new mapped-DTO denormalize call in a try/catch and rethrow as ToolException. That keeps the new code matching the docblock's promise. The pre-existing gap at the per-parameter path is real too, but it's not part of this diff. A separate follow-up issue for that works fine, no need to fix it here.
| } | ||
|
|
||
| /** | ||
| * @return array<string, mixed>|null | ||
| */ | ||
| private function buildParameters(string $className, string $methodName): ?array | ||
| { | ||
| $mapped = MappedToolArgument::forMethod(new \ReflectionMethod($className, $methodName)); | ||
| if (null === $mapped) { | ||
| return $this->factory->buildParameters($className, $methodName); | ||
| } | ||
|
|
||
| return $this->factory->buildProperties($mapped->className); | ||
| } |
There was a problem hiding this comment.
This buildParameters() helper is copy-pasted verbatim into ReflectionToolFactory.php too. Could live once, e.g. as a static helper on MappedToolArgument itself.
5901920 to
5778617
Compare
chr-hertel
left a comment
chr-hertel
left a comment
There was a problem hiding this comment.
Thanks for working on this @ineersa - I think it's a great feature!
Please revisit how services in ToolFactory implementations, JsonSchema\Factory and MappedToolArgument work together. There are a couple of symptoms that raise a flag for me here:
- in both
ToolFactoryimplementations theJsonSchema\Factoryis injected, but only forwarded as argument into a static method - it is an optional feature but feels like taking over the steering here
- Is
MappedToolArgumenta state object or a service? maybe we need to decouple - not sure
There was a problem hiding this comment.
Can we transform this into ObjectDescriberInterface implementation and encapsulate within the Factory basically?
There was a problem hiding this comment.
Yeah, this looks better now.
One thing to notice is factory wiring, if we want to provide ability to use this argument attribute by default, we need to wire describer into factories.
That was IMHO a bit too invasive, I've left previous commit, so you could check how it was looking.
As a tradeoff, application side now needs to configure factory with describer to use this feature properly - https://github.com/symfony/ai/pull/2510/changes#diff-e04a1b45e14764524023222d3e64d8f72f52e20b4d3ae6d7c2691c49fe787a83R528
carsonbot
changed the title
5778617 to
1f6830b
Compare
4 participants
#[MapToolArguments]attribute for exposing DTO properties at the schema root.