Skip to content

[DNM] Only send the delta on the partial tool call#2105

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

[DNM] Only send the delta on the partial tool call#2105
rumpl wants to merge 1 commit intodocker:mainfrom
rumpl:partial-tool-call

Conversation

@rumpl
Copy link
Member

@rumpl rumpl commented Mar 15, 2026

Do not merge, only opening to see if there are any test failures, some things fail for me locally

Signed-off-by: Djordje Lukic <djordje.lukic@docker.com>
@rumpl rumpl requested a review from a team as a code owner March 15, 2026 08:28
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.

Review Summary

Assessment: 🔴 CRITICAL

This PR introduces a critical bug in the partial tool call event merging logic that will cause function names to be lost during streaming.

Critical Issues Found

1 HIGH severity bug that will break streaming tool call displays
1 MEDIUM severity bug causing inconsistent event metadata

Details

The changes implement delta-based streaming for tool call arguments (sending only new tokens instead of the full accumulated string), but the merging logic in pkg/app/app.go has a critical flaw that will lose function names when consecutive events are merged.


Findings

ID: ev.ToolCall.ID,
Type: ev.ToolCall.Type,
Function: tools.FunctionCall{
Name: cmp.Or(next.ToolCall.Function.Name, latest.ToolCall.Function.Name),
Copy link

Choose a reason for hiding this comment

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

🔴 HIGH: Function name lost during consecutive PartialToolCallEvent merges

The cmp.Or(next.ToolCall.Function.Name, latest.ToolCall.Function.Name) logic is incorrect for merging delta events.

The Problem:

  • The first event ev contains the full function name (accumulated from streaming.go)
  • Subsequent events next have empty names because they're just argument deltas
  • cmp.Or returns the first non-empty value, preferring next.ToolCall.Function.Name (empty) over latest.ToolCall.Function.Name
  • This causes the function name to be lost after the first merge iteration

The Fix:
Use ev.ToolCall.Function.Name (from the first event in the merge chain) instead:

Name: ev.ToolCall.Function.Name,

This preserves the name learned from the first event throughout all subsequent merges.

Arguments: latest.ToolCall.Function.Arguments + next.ToolCall.Function.Arguments,
},
},
ToolDefinition: next.ToolDefinition,
Copy link

Choose a reason for hiding this comment

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

🟡 MEDIUM: Inconsistent AgentContext and ToolDefinition sources in merged event

The merged event uses AgentContext from the first event but ToolDefinition from the last event, creating an inconsistency.

The Issue:

AgentContext:   ev.AgentContext,      // from first event
ToolDefinition: next.ToolDefinition,  // from last event

This means the timestamp/context metadata comes from when the tool call was initiated, but the tool definition comes from the last delta event. While tool definitions are unlikely to change mid-stream, this violates consistency and could cause subtle bugs if tool definitions are updated dynamically.

Recommendation:
For consistency, use the tool definition from the first event:

ToolDefinition: ev.ToolDefinition,

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.

1 participant