fix router response to user#1381
Draft
iceljc wants to merge 1 commit into
Draft
Conversation
Contributor
PR Summary by QodoFix router replies by propagating function name/args through reasoners
AI Description
Diagram
High-Level Assessment
Files changed (5)
|
iceljc
marked this pull request as draft
July 16, 2026 22:10
Contributor
Code Review by Qodo
1. FunctionArgs used without validation
|
![qodo-code-review[bot]](https://avatars.githubusercontent.com/in/484649?s=60&v=4){kind=small}
Comment on lines
33
to
+37
| message.FunctionArgs = JsonSerializer.Serialize(inst); | ||
| if (!string.IsNullOrEmpty(inst.FunctionArgs)) | ||
| { | ||
| message.FunctionArgs = inst.FunctionArgs; | ||
| } |
Contributor
There was a problem hiding this comment.
1. functionargs used without validation 📘 Rule violation ☼ Reliability
The PR propagates raw LLM-provided response.FunctionArgs into message.FunctionArgs without validating it as JSON or providing a safe fallback. Downstream callbacks immediately JsonSerializer.Deserialize(...) message.FunctionArgs without guards, so malformed/empty FunctionArgs can throw and fail routing/user responses.
Agent Prompt
## Issue description
`inst.FunctionArgs` is treated as trusted JSON and copied into `message.FunctionArgs` without validation. Since routing callbacks deserialize `message.FunctionArgs` directly, malformed LLM output can throw and break the request instead of failing safely.
## Issue Context
- `inst.FunctionArgs` is assigned from `response.FunctionArgs` (LLM/provider boundary).
- `message.FunctionArgs` is then deserialized in multiple callbacks without try/catch or null/empty guards.
## Fix Focus Areas
- src/Infrastructure/BotSharp.Core/Routing/Reasoning/InstructExecutor.cs[33-37]
- src/Infrastructure/BotSharp.Core/Routing/Reasoning/HFReasoner.cs[56-67]
- src/Infrastructure/BotSharp.Core/Routing/Reasoning/NaiveReasoner.cs[60-71]
- src/Infrastructure/BotSharp.Core/Routing/Reasoning/OneStepForwardReasoner.cs[72-83]
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
Comment on lines
+64
to
67
| else if (!message.StopCompletion) | ||
| { | ||
| var state = _services.GetRequiredService<IConversationStateService>(); | ||
| var useStreamMsg = state.GetState("use_stream_message"); |
Contributor
There was a problem hiding this comment.
2. Stopcompletion not honored 🐞 Bug ≡ Correctness
In InstructExecutor.Execute, the invoked routing function runs on a cloned RoleDialogModel (msg), but the new guard uses message.StopCompletion (which is not updated), so InvokeAgent can still run even when the function intended to stop completion (e.g., response_to_user). This can produce an unintended extra router/agent completion and wrong final output flow.
Agent Prompt
### Issue description
`InstructExecutor.Execute` invokes the routing function using a cloned dialog `msg`, but then checks `message.StopCompletion` to decide whether to call `InvokeAgent`. Because `RoutingService.InvokeFunction` copies the stop flag back to the *passed* message instance (the clone), `message.StopCompletion` remains unchanged and routing continues even after a stop-intended function (e.g. `response_to_user`).
### Issue Context
- The executor creates `msg = RoleDialogModel.From(message, role: Function)` and invokes the function on `msg`.
- `response_to_user` sets `StopCompletion = true`.
- `RoutingService.InvokeFunction` copies `StopCompletion` back to its `message` parameter (the clone), not the original `message` held by the loop.
### Fix Focus Areas
- src/Infrastructure/BotSharp.Core/Routing/Reasoning/InstructExecutor.cs[39-80]
### Expected fix
- After `InvokeFunction(...)`, copy relevant termination state back (at minimum `message.StopCompletion = msg.StopCompletion`), or change the guard to check `msg.StopCompletion`.
- If `StopCompletion` is true after function execution, append an assistant response to `dialogs` (e.g., `dialogs.Add(RoleDialogModel.From(msg, role: AgentRole.Assistant, content: msg.Content))`) and return that response instead of calling `InvokeAgent`.
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
| var response = dialogs.Last(); | ||
|
|
||
| response.Instruction = inst; | ||
| response.StopCompletion = message.StopCompletion; |
Contributor
There was a problem hiding this comment.
3. Stopcompletion overwritten 🐞 Bug ≡ Correctness
In InstructExecutor.Execute, response.StopCompletion = message.StopCompletion can clear a StopCompletion flag already set on the actual response (e.g., by the LLM provider for max-token truncation), preventing InstructLoop from breaking. This can cause extra routing loops and unintended additional LLM/function calls.
Agent Prompt
### Issue description
`InstructExecutor.Execute` unconditionally overwrites `dialogs.Last().StopCompletion` with `message.StopCompletion`. This can clear a legitimate termination flag set on the response object itself, leading to extra iterations in `RoutingService.InstructLoop`.
### Issue Context
`InstructLoop` uses the returned `response.StopCompletion` to decide whether to break. Some providers set `StopCompletion = true` on assistant responses (e.g., max-token responses).
### Fix Focus Areas
- src/Infrastructure/BotSharp.Core/Routing/Reasoning/InstructExecutor.cs[76-80]
### Expected fix
- Do not overwrite `response.StopCompletion` from `message.StopCompletion`.
- If you need to propagate a stop condition from earlier stages, combine instead of replace (e.g., `response.StopCompletion = response.StopCompletion || message.StopCompletion`) *after* correctly propagating the stop flag from the function-executed message (see other finding).
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
No description provided.