Conversation
pranav100000
requested review from
brandonkachen,
charleslien and
jahooma
as code owners
| const messageHistoryBeforeStream = expireMessages( | ||
| agentState.messageHistory, | ||
| 'agentStep', | ||
| ) |
There was a problem hiding this comment.
I don't think this is necessary, this is run right before each step already (in runAgentStep)
| input: transformed ? transformed.input : input, | ||
| fromHandleSteps: false, | ||
| skipDirectResultPush: isXmlMode, | ||
| skipDirectResultPush: true, |
There was a problem hiding this comment.
Seems we should refactor to delete this param entirely if always true
| // Build message history from the pre-stream snapshot so tool_calls and | ||
| // tool_results are always appended in deterministic order. | ||
| agentState.messageHistory = buildArray<Message>([ | ||
| ...messageHistoryBeforeStream, | ||
| ...assistantMessages, | ||
| ...toolResultsToAddAfterStream, | ||
| ...errorMessages, | ||
| ]) |
There was a problem hiding this comment.
Conceptually, I think it does make more sense to do all the mutating of message history here, in one place.
| if (!excludeToolFromMessageHistory && !params.skipDirectResultPush) { | ||
| agentState.messageHistory.push(toolResult) | ||
| } |
There was a problem hiding this comment.
In the new paradigm, we can delete this if and
agentState.messageHistory.push(toolResult).
We might want to rename toolResultsToAddAfterStream to toolResultsToAddToMessageHistory, since all tool results are added after stream and these are the ones included in the message history in particular
jahooma
left a comment
jahooma
left a comment
There was a problem hiding this comment.
Thanks Pranav! This is a tricky bug and our code here is kinda bad haha.
Made some comments, but the thrust of the change seems good.
If you make these fixes, I'll test & merge it!
|
Made a few more changes and merged as this PR: #431 Thanks! |
2 participants
No description provided.