Skip to content
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

Merged
merged 6 commits into from
Oct 16, 2024

Conversation

shethaadit
Copy link
Contributor

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 a System.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

  • Enhanced the validation logic for FunctionArgumentsUpdate by adding a check for both null and empty states, ensuring that invalid data is filtered out before processing.
  • Refined the tool call processing flow to handle complex argument updates more gracefully.
  • Improved error handling within the TrackStreamingToolingUpdate method, preventing memory issues and system crashes when receiving argument updates from external APIs.
  • The change introduces a more robust approach to managing function argument updates and ensures tool calls are stable even in edge cases involving empty inputs.

Unit Test Details

  • Created unit tests to validate the TrackStreamingToolingUpdate method when handling null values in FunctionArgumentsUpdate, ensuring it behaves correctly under various scenarios.
  • Added a regression test to ensure that if the null check is removed in future changes, the test case will fail, providing an extra layer of protection against potential issues.

Contribution Checklist

  • [Y] Code builds and passes tests.
  • [Y] Unit tests added for edge case scenarios.
  • [Y] Changes have been tested to ensure they do not break existing functionality.
  • [N/A] Documentation updated where necessary.

@shethaadit shethaadit requested a review from a team as a code owner October 15, 2024 05:35
@markwallace-microsoft markwallace-microsoft added .NET Issue or Pull requests regarding .NET code kernel Issues or pull requests impacting the core kernel labels Oct 15, 2024
@github-actions github-actions bot changed the title Fix: Enhance Function Argument Validation to Prevent Null Argument Exceptions in Tool Calls. .Net: Fix: Enhance Function Argument Validation to Prevent Null Argument Exceptions in Tool Calls. Oct 15, 2024
@RogerBarreto
Copy link
Member

@shethaadit Please agree and we can merge it.

@shethaadit
Copy link
Contributor Author

@microsoft-github-policy-service agree company="Microsoft"

@RogerBarreto RogerBarreto added this pull request to the merge queue Oct 15, 2024
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to failed status checks Oct 15, 2024
@shethaadit
Copy link
Contributor Author

@microsoft-github-policy-service agree company="Microsoft"

@RogerBarreto RogerBarreto added this pull request to the merge queue Oct 16, 2024
Merged via the queue into microsoft:main with commit 8f8c768 Oct 16, 2024
15 checks passed
@@ -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)
Copy link
Member

@stephentoub stephentoub Oct 17, 2024

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
kernel Issues or pull requests impacting the core kernel .NET Issue or Pull requests regarding .NET code
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

.Net: Function Arguments Is Empty
4 participants