-
Notifications
You must be signed in to change notification settings - Fork 3.4k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
.Net: Fix: Enhance Function Argument Validation to Prevent Null Argument Exceptions in Tool Calls. #9273
.Net: Fix: Enhance Function Argument Validation to Prevent Null Argument Exceptions in Tool Calls. #9273
Conversation
dotnet/src/Connectors/Connectors.OpenAI.UnitTests/Core/OpenAIFunctionToolCallTests.cs
Show resolved
Hide resolved
@shethaadit Please agree and we can merge it. |
@microsoft-github-policy-service agree company="Microsoft" |
@microsoft-github-policy-service agree company="Microsoft" |
@@ -121,7 +121,7 @@ internal static void TrackStreamingToolingUpdate( | |||
} | |||
|
|||
// Ensure we're tracking the function's arguments. | |||
if (update.FunctionArgumentsUpdate is not null) | |||
if (update.FunctionArgumentsUpdate is not null && !update.FunctionArgumentsUpdate.ToMemory().IsEmpty) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For my edification, why is this additional clause required? Is it trying to be an optimization, or is it fixing an issue? Based on a cursory look at the code, for the condition that clause is guarding, I'd have expected arguments.Append(update.FunctionArgumentsUpdate.ToString()) to simply be the equivalent of arguments.Append(""), since BinaryData.ToString() just does Encoding.UTF8.GetString on the supplied bytes, and passing in an empty set of bytes will lead that to return an empty string.
Motivation and Context
1. Why is this change required?
This change is required to prevent null pointer exceptions caused by invalid or empty function argument updates, which could lead to system crashes or unexpected behavior during runtime.
2. What problem does it solve?
It addresses the issue where the
FunctionArgumentsUpdate
was not being properly validated, causing the Semantic Kernel to throw aSystem.ArgumentNullException
. This update ensures proper validation of function arguments before processing.3. What scenario does it contribute to?
It improves the stability and reliability of tool call functions, particularly when interacting with external services like OpenAI, ensuring that memory operations are only executed on valid and non-empty inputs.
This PR resolves #9212.
Description
FunctionArgumentsUpdate
by adding a check for both null and empty states, ensuring that invalid data is filtered out before processing.TrackStreamingToolingUpdate
method, preventing memory issues and system crashes when receiving argument updates from external APIs.Unit Test Details
TrackStreamingToolingUpdate
method when handlingnull
values inFunctionArgumentsUpdate
, ensuring it behaves correctly under various scenarios.null
check is removed in future changes, the test case will fail, providing an extra layer of protection against potential issues.Contribution Checklist