Skip to content

Remove the tool call from the tool call response event#2163

Open
rumpl wants to merge 1 commit intodocker:mainfrom
rumpl:remove-tool-call
Open

Remove the tool call from the tool call response event#2163
rumpl wants to merge 1 commit intodocker:mainfrom
rumpl:remove-tool-call

Conversation

@rumpl
Copy link
Member

@rumpl rumpl commented Mar 18, 2026

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

@rumpl rumpl requested a review from a team as a code owner March 18, 2026 22:18
dgageot
dgageot previously approved these changes Mar 18, 2026
docker-agent[bot]

This comment was marked as outdated.

@rumpl
Copy link
Member Author

rumpl commented Mar 18, 2026

/review

Copy link

@docker-agent docker-agent bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Assessment: 🟡 NEEDS ATTENTION

Found 2 medium-severity issues in the tool call argument tracking implementation.

}

case *runtime.ToolCallEvent:
toolCallArgs[e.ToolCall.ID] = e.ToolCall.Function.Arguments
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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
    }

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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>
@rumpl rumpl force-pushed the remove-tool-call branch from 90088ab to 0998309 Compare March 18, 2026 22:34
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants