fix: adding fixHistory logic for agentic Chat #1050
Conversation
| this.#features.logging.debug('No history message found, but new user message has tool results.') | ||
| newUserMessage.userInputMessage.userInputMessageContext.toolResults = undefined | ||
| // tool results are empty, so content must not be empty | ||
| newUserMessage.userInputMessage.content = 'Conversation history was too large, so it was cleared.' |
There was a problem hiding this comment.
Is this customer-facing notification and Is it correctly explains current state?
And what was cleared exactly - tool use or whole history? How I read it, it's whole history was cleared, which sounds scary
There was a problem hiding this comment.
If this is customer facing, I think it should be more clear
There was a problem hiding this comment.
This is not a customer facing notification.
| if (message.toolUses) { | ||
| try { | ||
| for (const toolUse of message.toolUses) { | ||
| count += JSON.stringify(toolUse).length |
There was a problem hiding this comment.
This function is called inside of tool use loop, which already set to run for 100 iterations. I don't know if you've tested it, but I suspect running bunch of JSON.stringify on whole conversation history every time will impact performance quite badly. Did you test the performance of this code on large dataset in history?
There was a problem hiding this comment.
As of now, I was testing the functionality to work first. Right now did not see it causing any performance issue. Reference code: https://github.com/aws/aws-toolkit-vscode/blob/a597cd2817bf4bf861f947aab25af8fddcc01ff5/packages/core/src/shared/db/chatDb/chatDb.ts#L419
| if (currentMessage) { | ||
| this.#chatHistoryDb.fixHistory(tabId, currentMessage, session.conversationId ?? '') | ||
| } |
There was a problem hiding this comment.
Shouldn't this be outside the agent loop? It doesn't influence the current loop since we add history in memory below (line 375) instead of in the db during the loop.
There was a problem hiding this comment.
I believe this should be done before we call generateAssistantResponse. The history needs to be fixed before sending the request
This reverts commit 4a7ad34.
Problem
Solution
License
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.