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_weather becomes 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 ParseFullyQualifiedFunctionName method 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 ParseFullyQualifiedFunctionName helper 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 named send_email in a plugin named tools would be sent as tools_send_email; the heuristic would correctly split it, but a plugin named my_tools_v2 with function ReadFile would be sent as my_tools_v2_ReadFile and 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 because FunctionName.Parse always 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_ReadFile splits 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_weather with 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