Fix PluginName empty when using Ollama/Gemini with AddFromObject (#13516)#13672
Fix PluginName empty when using Ollama/Gemini with AddFromObject (#13516)#13672aayushbaluni wants to merge 1 commit intomicrosoft:mainfrom
Conversation
…rosoft#13516) FunctionCallContentBuilder.Parse used default separator '-' but Ollama and Gemini connectors use '_' (FullyQualifiedAIFunction), and FunctionChoiceBehavior uses '.'. Try separators '.', '_', '-' when parsing fully-qualified function names to support all connector formats.
|
@microsoft-github-policy-service agree |
chetantoshniwal
left a comment
There was a problem hiding this comment.
Automated Code Review
Reviewers: 4 | Confidence: 86%
✗ Correctness
The new ParseFullyQualifiedFunctionName method introduces a correctness regression: it greedily tries underscore and hyphen as plugin-function separators, meaning any function name naturally containing those characters (e.g., 'get_weather' with no plugin) will be falsely split into a plugin and function name. The PluginName null-check on line 185 provides no real protection because FunctionName.Parse will always yield a non-null PluginName when the separator is found in the string. Additionally, the new method is inserted between the XML doc comments of TrackStreamingFunctionCallUpdate and the method itself, breaking the documentation association for that method.
✗ Security Reliability
The new ParseFullyQualifiedFunctionName method introduces ambiguous function name parsing by trying underscore and hyphen as plugin-function separators. Since underscores and hyphens are extremely common in function names themselves (e.g.,
get_weather,read_file), any such function without a plugin prefix will be incorrectly split—get_weatherbecomes plugin=get, function=weather. This can cause silent misrouting of function calls or resolution failures at runtime, which is both a reliability and a potential security concern (invoking the wrong function). The greedy first-match-wins approach on common characters significantly broadens the attack surface if an adversarial model response can influence function name parsing.
✗ Test Coverage
The new
ParseFullyQualifiedFunctionNamemethod supports three separators (".", "_", "-") plus a fallback path when none match, but the tests only cover dot and underscore. The hyphen separator path and the no-separator fallback path are untested. Additionally, there is no test for ambiguous underscore names (e.g.,my_plugin_MyFunction) where the first underscore split may produce an unexpected plugin/function split, nor for names containing multiple separator types (e.g.,time.Read_File) to verify that dot takes priority.
✗ Design Approach
The change introduces a heuristic
ParseFullyQualifiedFunctionNamehelper that tries three separators in order (".", "_", "-") to split a fully-qualified AI function name into plugin and function parts. While this fixes the reported Ollama/Gemini underscore case, the approach is fundamentally ambiguous: the same string can be parsed differently depending on which separator appears first, and the parser has no way to distinguish a separator character that is part of a plugin or function name from an actual delimiter. For example, a function namedsend_emailin a plugin namedtoolswould be sent astools_send_email; the heuristic would correctly split it, but a plugin namedmy_tools_v2with functionReadFilewould be sent asmy_tools_v2_ReadFileand incorrectly parsed as plugin=my, function=tools_v2_ReadFile. The '-' separator is especially risky because hyphens are common in identifiers. The root cause is that different AI connectors format names differently on the way out, yet the parser on the way back in has no record of which separator was used. The correct fix is to either (1) make each connector pass its separator into the builder so it is used for parsing, or (2) resolve the ambiguity by looking up the incoming name against the kernel's registered function registry rather than guessing from the string alone.
Flagged Issues
- Heuristic separator guessing is fundamentally ambiguous: any bare function name containing '_' or '-' (e.g.,
get_weather,get-weather) will be falsely split into a plugin and function name becauseFunctionName.Parsealways produces a non-null PluginName when the separator is found. This causes silent misrouting of function calls and can invoke unintended functions. The same problem affects plugin names containing separators (e.g.,my_tools_v2_ReadFilesplits as plugin=my, function=tools_v2_ReadFile). - Critical test gaps: the hyphen separator branch, the no-separator fallback path (plain function name like
ReadFile), and bare function names containing separator characters (e.g.,get_weatherwith no plugin) are all untested. The current tests only validate the happy path where a plugin prefix genuinely exists.
Suggestions
- Thread the separator from the connector layer into
FunctionCallContentBuilder(e.g., via a constructor parameter or per-call context) so parsing is unambiguous. Alternatively, resolve function names by looking them up against the kernel's registered function registry rather than guessing from the string. - If multi-separator guessing must stay, add a validation step that checks the parsed plugin/function actually exists in the kernel before accepting the split; fall back to treating the whole string as a function name if not. As a quick fix, at minimum remove '-' from the candidate separators and document the known ambiguity.
- Move the new method above the XML doc block or below TrackStreamingFunctionCallUpdate to avoid orphaning the
<param>documentation for that method. - Add comprehensive test coverage: (a) bare function name with underscores and no plugin (e.g.,
get_weather), (b) hyphen separator (e.g.,time-ReadFile), (c) plain function name with no separator (e.g.,ReadFile), (d) name with multiple separator types (e.g.,time.Read_File) to verify priority, (e) ambiguous multi-underscore name (e.g.,my_plugin_MyFunction) to document the first-match behavior.
Automated review by chetantoshniwal's agents
| foreach (var separator in new[] { ".", "_", "-" }) | ||
| { | ||
| if (fullyQualifiedName.Contains(separator, StringComparison.Ordinal)) | ||
| { | ||
| var parsed = FunctionName.Parse(fullyQualifiedName, separator); |
There was a problem hiding this comment.
Bug: This separator-guessing loop will falsely split any bare function name containing ., _, or -. For example, FunctionName.Parse("get_weather", "_") produces PluginName="get", Name="weather", and the null check on line 185 passes because a non-null PluginName is always produced when the separator exists. Names with multiple underscores (e.g., my_tools_v2_ReadFile) split incorrectly as plugin=my, function=tools_v2_ReadFile. The separator must come from the connector that formatted the name, not be guessed from the string.
| foreach (var separator in new[] { ".", "_", "-" }) | |
| { | |
| if (fullyQualifiedName.Contains(separator, StringComparison.Ordinal)) | |
| { | |
| var parsed = FunctionName.Parse(fullyQualifiedName, separator); | |
| // TODO: accept the separator used by the originating connector instead of guessing. | |
| private static FunctionName ParseFullyQualifiedFunctionName(string fullyQualifiedName) | |
| { | |
| return FunctionName.Parse(fullyQualifiedName); | |
| } |
| if (fullyQualifiedName.Contains(separator, StringComparison.Ordinal)) | ||
| { | ||
| var parsed = FunctionName.Parse(fullyQualifiedName, separator); | ||
| if (parsed.PluginName is not null) |
There was a problem hiding this comment.
Security: The parsed.PluginName is not null check is insufficient—any name containing the separator produces a non-null PluginName. An adversarial model response like admin_DeleteAllData would be parsed as plugin=admin, function=DeleteAllData, potentially routing to an unintended privileged function.
| } | ||
| } | ||
| } | ||
|
|
There was a problem hiding this comment.
Including - as a candidate separator will silently mis-parse any hyphenated function name as a plugin-qualified call. Hyphens are far more common in identifiers than as SK plugin separators and should not be probed here.
|
|
||
| /// <summary> | ||
| /// Parses a fully-qualified function name into plugin and function parts. | ||
| /// Tries multiple separators (".", "_", "-") since different AI connectors use different formats. | ||
| /// </summary> | ||
| private static FunctionName ParseFullyQualifiedFunctionName(string fullyQualifiedName) | ||
| { | ||
| foreach (var separator in new[] { ".", "_", "-" }) | ||
| { | ||
| if (fullyQualifiedName.Contains(separator, StringComparison.Ordinal)) | ||
| { | ||
| var parsed = FunctionName.Parse(fullyQualifiedName, separator); | ||
| if (parsed.PluginName is not null) | ||
| { | ||
| return parsed; | ||
| } | ||
| } | ||
| } | ||
|
|
||
| return FunctionName.Parse(fullyQualifiedName); | ||
| } | ||
|
|
There was a problem hiding this comment.
Structural issue: inserting the new method here splits the XML <param> doc comments (lines 170-172) from their target method TrackStreamingFunctionCallUpdate (now at line 195), orphaning the parameter documentation. Move this method above the doc block or below TrackStreamingFunctionCallUpdate.
| } | ||
|
|
||
| [Fact] | ||
| public void ItShouldParseUnderscoreSeparatorForOllamaAndGeminiConnectors() |
There was a problem hiding this comment.
The test name time_ReadFile is unambiguous because time is clearly a plugin. Add a test for a bare function name containing underscores (e.g., read_file with no plugin) to verify it is NOT incorrectly split into plugin=read, name=file.
| Assert.Equal("ReadFile", functionCall.FunctionName); | ||
| } | ||
|
|
||
| private static StreamingChatMessageContent CreateStreamingContentWithFunctionCallUpdate(int choiceIndex, int functionCallIndex, string? callId, string? name, string? arguments, int requestIndex = 0) |
There was a problem hiding this comment.
Missing test coverage: (1) the hyphen separator path is untested—add a test with e.g. "time-ReadFile" asserting PluginName="time", FunctionName="ReadFile"; (2) the fallback FunctionName.Parse(fullyQualifiedName) path is untested—add a test with a plain name like "ReadFile" asserting PluginName is null and FunctionName is "ReadFile".
Description
Fixes #13516
FunctionCallContentBuilderusedFunctionName.Parse(fqn)with the default separator-when buildingFunctionCallContentfrom streaming updates. However, different AI connectors use different separators:_(e.g.time_ReadFile)_-.When using
kernel.Plugins.AddFromObject(plugin, pluginName: "time")with Ollama, the model returnstime_ReadFilebut the builder parsed with-, producingpluginName=nullandfunctionName="time_ReadFile".Changes
ParseFullyQualifiedFunctionNamehelper that tries separators.,_,-when parsingRoot Cause
FunctionCallContentBuilder.Build()calledFunctionName.Parse(fqn)without a separator, defaulting to-. Connectors using_or.produced unparseable names, leavingPluginNameempty.Made with Cursor