-
-
Notifications
You must be signed in to change notification settings - Fork 102
[Agent] Dispatch more tool call events #709
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
/** | ||
* Dispatched after successfully invoking a tool. | ||
*/ | ||
final readonly class ToolCallSuccess |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
either ToolCallSuccessful
or SuccessfulToolCall
? Same for the failed one.
WDYT @chr-hertel ?
Thanks for tackling this topic, can you please add some docs? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think in general this totally makes sense, and I'd only see some fine-tuning here.
- I think it's a bit odd to me having basically two events with the same payload dispatched line after line, referring to
ToolCallArgumentsResolved
andExecuteToolCall
- do i miss something here? - I would propose to move to an event-style of naming, and we're half way there
proposed changes:
- drop
ExecuteToolCall
- rename
ToolCallSuccess
toToolCallSucceeded
@chr-hertel I renamed the success event and dropped ExecuteToolCall. |
a8ec69a
to
e17fbd5
Compare
e17fbd5
to
36079d2
Compare
Thank you @franzwilding, I rebased your PR to get rid of the merge commit 👍 |
Nice one @franzwilding - thank you! 👍 |
This PR dispatches three new events around tool calling.