[Agent Builder] do not store filestore tool calls in filestore#254850
[Agent Builder] do not store filestore tool calls in filestore#254850pgayvallet wants to merge 9 commits intoelastic:mainfrom
Conversation
|
/ci |
pgayvallet
added
release_note:skip
|
/ci |
|
/ci |
|
/ci |
| const managerSummarizer = toolManager.getSummarizer(toolCall.tool_id); | ||
| if (managerSummarizer) { | ||
| const summarizedResults = managerSummarizer(toolCall); | ||
| return summarizedResults ?? undefined; | ||
| } |
There was a problem hiding this comment.
Internal tools such as the filestore and attachment tools aren't registered in the tool registry, so that whole logic of result summarization wasnt working properly for them. Had to adapt a bunch of things, and use the tool manager as a second source of truth for the tool definitions.
Note that this also fixes a bug with attachment tools, because they currently define summarizeToolReturn which aren't used without that PR.
| return toolReturn.results.map((result) => ({ | ||
| tool_result_id: result.tool_result_id, | ||
| type: ToolResultType.other, | ||
| data: { | ||
| comment: 'Tool result removed from the discussion, call the tool again to access the data', | ||
| }, | ||
| })); |
There was a problem hiding this comment.
The new placeholder data we use for all filestore tool calls. I kept it fairly simple.
| /** | ||
| * Gets the summarizer function for a tool by its internal tool ID. | ||
| * Returns undefined if the tool is not found or has no summarizer. | ||
| */ | ||
| public getSummarizer(toolId: string): ToolReturnSummarizerFn | undefined { | ||
| return this.executableTools.get(toolId)?.summarizeToolReturn; | ||
| } |
There was a problem hiding this comment.
Related to previous comments - the tool manager is the service knowing about non-registry tools, so I had to adapt it to keep track of its tools to access the summarizers
|
/ci |
| ...getConversationAttachmentsSystemMessages( | ||
| params.processedConversation.versionedAttachmentPresentation | ||
| ), |
There was a problem hiding this comment.
Not directly related to the PR, but I took the opportunity to clean how the attachment prompt is being added
💚 Build Succeeded
Metrics [docs]
History
cc @pgayvallet |
|
@elasticmachine merge upstream |
3 participants
Summary
Context: slack discussion
Storing filestore (and attachment) tool call results in the filestore doesn't bring anything at best (given the data is already in the store from the initial tool call), and is confusing for the LLM at worse.
This PR changes that behavior, so that:
filestore.*orattachments.*tools aren't persisted in the storagefilestore.*tools summarization is now using a simple placeholder instead of a link to the filestore.