-
-
Notifications
You must be signed in to change notification settings - Fork 612
fix(routing): normalize function name, fix length-truncated response … #1291
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
Conversation
…handling, add DiagnosticHelper
Review Summary by QodoNormalize function names, handle length-truncated responses, and add diagnostic utilities
WalkthroughsDescription• Add NormalizeFunctionName() extension method to handle LLM-generated function names with namespace/agent prefixes • Create DiagnosticHelper utility class for stack trace debugging • Fix length-truncated response handling across multiple chat completion providers • Consolidate function name normalization logic across 6 provider implementations • Improve error logging with diagnostic stack trace information Diagramflowchart LR
A["LLM Response with<br/>Prefixed Function Names"] -->|NormalizeFunctionName| B["Cleaned Function Names"]
C["Length-Truncated<br/>Responses"] -->|ChatFinishReason.Length| D["Handled with<br/>StopCompletion Flag"]
E["Function Execution<br/>Failures"] -->|DiagnosticHelper| F["Enhanced Error Logs<br/>with Stack Trace"]
B --> G["6 Chat Providers<br/>Updated"]
D --> G
F --> G
File Changes1. src/Infrastructure/BotSharp.Abstraction/Utilities/DiagnosticHelper.cs
|
Code Review by Qodo
1. Stack trace logged in error
|
| message.StopCompletion = true; | ||
| message.Content = $"Can't find function implementation of {name}."; | ||
| _logger.LogError(message.Content); | ||
| _logger.LogError($"{message.Content}, stackInfo:{DiagnosticHelper.GetCurrentStackTrace()}"); | ||
| return false; |
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.
1. Stack trace logged in error 📘 Rule violation ⛨ Security
• The new error log appends a full stack trace via DiagnosticHelper.GetCurrentStackTrace() when a function implementation is missing. • Stack traces can contain sensitive internal details (method names, paths, code locations) and the log line is also unstructured, making it harder to audit safely. • This increases the risk of sensitive information exposure through logs and reduces log usability for monitoring/auditing.
Agent Prompt
## Issue description
The code logs a full stack trace as part of an interpolated error message. This can leak internal details into logs and is not structured logging.
## Issue Context
Secure logging requires logs to be structured and to avoid sensitive/internal details in log output.
## Fix Focus Areas
- src/Infrastructure/BotSharp.Core/Routing/RoutingService.InvokeFunction.cs[19-22]
- src/Infrastructure/BotSharp.Abstraction/Utilities/DiagnosticHelper.cs[12-15]
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
| public static string? NormalizeFunctionName(this string? functionName) | ||
| { | ||
| if (string.IsNullOrEmpty(functionName)) | ||
| { | ||
| return functionName; | ||
| } | ||
|
|
||
| if (functionName.Contains('.')) | ||
| { | ||
| return functionName.Split('.').Last(); | ||
| } | ||
|
|
||
| if (functionName.Contains('/')) | ||
| { | ||
| return functionName.Split('/').Last(); | ||
| } |
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.
2. Normalization can yield invalid name 🐞 Bug ✓ Correctness
• NormalizeFunctionName can return empty string or whitespace (e.g., inputs like "Agent." or "Agent/ ") because it doesn’t trim or remove empty segments. • Multiple call sites now assign the normalized value back to FunctionName/args.Function without validation, increasing the chance of passing an empty/invalid function name downstream. • Function execution is resolved by exact name equality; an empty/whitespace name will fail lookup and surface as “Can’t find function implementation …”, making failures harder to attribute to malformed LLM output.
Agent Prompt
## Issue description
`NormalizeFunctionName` can normalize malformed names into empty/whitespace strings (e.g., trailing `.` or `/`, leading/trailing spaces). Those values then fail downstream exact-match lookups for executors.
## Issue Context
Function execution is resolved via `x.Name == functionName` (exact equality). Call sites now overwrite the function name with the normalized value without validating it.
## Fix Focus Areas
- src/Infrastructure/BotSharp.Abstraction/Utilities/StringExtensions.cs[74-92]
- src/Infrastructure/BotSharp.Core/Routing/Executor/FunctionExecutorFactory.cs[8-18]
## Suggested direction
- Update `NormalizeFunctionName` to:
- use `string.IsNullOrWhiteSpace` for the initial guard
- `Trim()` the input
- split on both `.` and `/` (e.g., via `Split(new[]{'.','/'}, StringSplitOptions.RemoveEmptyEntries)`)
- return the last non-empty segment (trimmed)
- if the normalized token is empty, return `null` (or original) so callers can handle it
- Optionally add a small validation at call sites: if normalized is null/whitespace, do not overwrite and/or set a clear error explaining malformed tool name.
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
|
reviewed |
fix(routing): normalize function name, fix length-truncated response handling, add DiagnosticHelper