fix: handle mixed text and function call responses in shim #437
Conversation
- Fix candidateToShimCandidate to detect function calls in mixed responses - Pass through original response when function calls are present - Prevent premature conversation termination - Add comprehensive tests for mixed response handling Fixes GoogleCloudPlatform#427
noahlwest
left a comment
noahlwest
left a comment
There was a problem hiding this comment.
This seems like a good change to me, though I have a couple of questions:
Description says "Add comprehensive tests for mixed response handling", is this something you intended to add or was it accidentally left in the description?
Also, were you able to verify this by recreating the issue from #427?
|
ohh sorry, I created some test files but I didn't add tests when committing, You're right, that description was misleading. I should remove it. Yes, I verified it the candidateToShimCandidate function was erroring out on function calls (non-text parts), causing mixed responses to fail with "no text part found in candidate" and skip tool execution. |
|
The change fixes the bug where function calls were not executed when combined with text responses. Screen.Recording.2025-08-01.at.12.53.05.AM.mov |
| // Instead, pass through the original response | ||
| klog.Infof("Warning: there are non-text parts FunctionCall in the response, returning concatenation of all text parts.") | ||
| // Return the original response without shim conversion | ||
| yield(response, nil) |
There was a problem hiding this comment.
If I understand correctly,
in case of tool-use-shim, the API does not return toolCalls, so we accumulate all the text and then parse the text to convert into toolcall. So the change here will simply return text response to the layer above.
@Vinay-Khanagavi I don't think the original issue involved |
There was a problem hiding this comment.
Pull Request Overview
This PR fixes the handling of mixed text and function call responses in the conversation shim by preventing premature error termination when function calls are present alongside text.
- Replaces error throwing with function call detection and original response passthrough
- Adds logging to warn about non-text parts in responses
- Ensures function calls don't cause conversation failures
| } else if calls, ok := part.AsFunctionCalls(); ok && len(calls) > 0 { | ||
| // If we encounter function calls, we should not use the shim | ||
| // Instead, pass through the original response | ||
| klog.Infof("Warning: there are non-text parts FunctionCall in the response, returning concatenation of all text parts.") |
There was a problem hiding this comment.
The log message is misleading. It states 'returning concatenation of all text parts' but the code actually returns the original response without concatenation.
| klog.Infof("Warning: there are non-text parts FunctionCall in the response, returning concatenation of all text parts.") | |
| klog.Infof("Warning: there are non-text parts FunctionCall in the response, returning the original response without concatenation.") |
| } else if calls, ok := part.AsFunctionCalls(); ok && len(calls) > 0 { | ||
| // If we encounter function calls, we should not use the shim | ||
| // Instead, pass through the original response | ||
| klog.Infof("Warning: there are non-text parts FunctionCall in the response, returning concatenation of all text parts.") |
There was a problem hiding this comment.
Grammar issue: 'FunctionCall' should be 'function calls' for better readability.
| klog.Infof("Warning: there are non-text parts FunctionCall in the response, returning concatenation of all text parts.") | |
| klog.Infof("Warning: there are non-text parts (function calls) in the response, returning concatenation of all text parts.") |
| // Instead, pass through the original response | ||
| klog.Infof("Warning: there are non-text parts FunctionCall in the response, returning concatenation of all text parts.") | ||
| // Return the original response without shim conversion | ||
| yield(response, nil) |
There was a problem hiding this comment.
The accumulated text buffer is discarded when function calls are detected. Consider whether any accumulated text should be preserved or if this is the intended behavior.
3 participants
Fixes #427