[Agent] Code grooming around tool call #1010
Conversation
lyrixx
commented
Nov 27, 2025
lyrixx
commented
| Q | A |
|---|---|
| Bug fix? | no |
| New feature? | no |
| Docs? | no |
| Issues | |
| License | MIT |
lyrixx
requested review from
Nyholm,
OskarStark and
chr-hertel
as code owners
carsonbot
added
Agent
| } | ||
|
|
||
| private function handleToolCallsCallback(Output $output): \Closure | ||
| private function handleToolCallsCallback(Output $output, ToolCallResult $result, ?AssistantMessage $streamedAssistantResponse = null): ResultInterface |
There was a problem hiding this comment.
I reduced the indirection. This reduce the depth in the stack.
|
|
||
| /** | ||
| * @param class-string $reference | ||
| * @param class-string $className |
There was a problem hiding this comment.
Let's use only one name for a "thing". Line 30, this is called className.
I think it's better to be consistant, it's easier to understand
There was a problem hiding this comment.
doesn't have to be a class tho, right?
There was a problem hiding this comment.
I'm not sure to understand. What could it be, if it's not a class, but the type is class-string?
| * List of executable tools. | ||
| * | ||
| * @var list<mixed> | ||
| * @var list<object> |
There was a problem hiding this comment.
We call ::class on item. So it must be an object. See linne 69
There was a problem hiding this comment.
can we extract this as a small bugfix please?
| $this->tools = $tools instanceof \Traversable ? iterator_to_array($tools) : $tools; | ||
| } | ||
|
|
||
| public function getTools(): array |
There was a problem hiding this comment.
There is something a bit hard to understand.
getTools does not return tool, but tools metadata !
I would like to rebame Tool to ToolMetadata, getTools() to getToolsMetadata()
It will really ease on-boarding I guess.
And it will fix inconsistency. For example :
private function getMetadata(ToolCall $toolCall): Tool
lyrixx
force-pushed
the
agent-tool-review
branch
from
8cd7562 to
42f101c
Compare
|
PR title: Tall or Tool? |
lyrixx
changed the title
|
Ouch, too many typos 😅 . I edited the PR title. |
chr-hertel
left a comment
chr-hertel
left a comment
There was a problem hiding this comment.
fine after this small gotcha
|
and the PHPStan issue looks real - wonder if that's worth the trouble |
lyrixx
force-pushed
the
agent-tool-review
branch
from
42f101c to
738c641
Compare
There was a real issue with PHPStan. Since the code is now better, PHPStan found the typo. I fixed it. Pr rebases, and cleaned. Ready for review Note there is a pending question about a bigger cleaning. It would happen in a new PR, But I would love to get an answer. |
| * role: 'assistant', | ||
| * content: ?string, | ||
| * tool_calls: array{ | ||
| * tool_calls: list<array{ |
There was a problem hiding this comment.
this looks like a bug fix to me, which deserves a separate PR
…ltConverter` (lyrixx) This PR was merged into the main branch. Discussion ---------- [Platform][OpenAI] Fix `$choice` argument type in `Gpt\ResultConverter` | Q | A | ------------- | --- | Bug fix? | yes | New feature? | no | Docs? | no | Issues | | License | MIT See #1010 (comment) Commits ------- 4d59873 [Platform] Fix `$choice` argument type in Gpt\ResultConverter
This PR was merged into the main branch. Discussion ---------- [Agent] Fix type of `Toolbox::$tools` | Q | A | ------------- | --- | Bug fix? | yes | New feature? | no | Docs? | no | Issues | | License | MIT see #1010 (comment) Commits ------- 0a90585 [Agent] Fix type of Toolbox::$tools
lyrixx
force-pushed
the
agent-tool-review
branch
from
738c641 to
d6b873e
Compare
4 participants
