Remove the tool call from the tool call response event#2163
Remove the tool call from the tool call response event#2163rumpl wants to merge 1 commit intodocker:mainfrom
Conversation
|
/review |
| } | ||
|
|
||
| case *runtime.ToolCallEvent: | ||
| toolCallArgs[e.ToolCall.ID] = e.ToolCall.Function.Arguments |
There was a problem hiding this comment.
Memory leak: toolCallArgs map not cleaned on tool rejection
The toolCallArgs map is populated when a ToolCallEvent is received (line 417), but if the tool call is rejected during the confirmation step (line 410-415), no ToolCallResponseEvent is ever emitted. This means the map entry is never deleted (line 429) and persists for the duration of the agent turn.
While the map is recreated per-turn, this is still a memory leak within long-running turns that process many tool calls with rejections.
Recommendation: Add cleanup in the rejection path:
case *runtime.ToolCallConfirmationEvent:
resumeType, err := a.handleToolCallConfirmation(ctx, acpSess, e)
if err != nil {
return err
}
if resumeType == ResumeTypeReject {
delete(toolCallArgs, e.ToolCall.ID) // Clean up rejected tool call args
}There was a problem hiding this comment.
This is not true, even when a tool call is rejected we send back a tool call result to the LLM telling it that ... the tool call was rejected.
This tool call was already sent in previous events, no need to re-send it especially since it can be big for certain calls like write_file. We do need the tool call id though so that clients can still find the tool call that this is the result for Signed-off-by: Djordje Lukic <djordje.lukic@docker.com>
This tool call was already sent in previous events, no need to re-send it especially since it can be big for certain calls like write_file. We do need the tool call id though so that clients can still find the tool call that this is the result for