[DNM] Only send the delta on the partial tool call#2105
[DNM] Only send the delta on the partial tool call#2105rumpl wants to merge 1 commit intodocker:mainfrom
Conversation
Signed-off-by: Djordje Lukic <djordje.lukic@docker.com>
There was a problem hiding this comment.
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), |
There was a problem hiding this comment.
🔴 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
evcontains the full function name (accumulated from streaming.go) - Subsequent events
nexthave empty names because they're just argument deltas cmp.Orreturns the first non-empty value, preferringnext.ToolCall.Function.Name(empty) overlatest.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, |
There was a problem hiding this comment.
🟡 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 eventThis 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,
Do not merge, only opening to see if there are any test failures, some things fail for me locally