Skip to content

fix router response to user #1381

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.

Sign up for GitHub

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

Draft
iceljc wants to merge 1 commit into
base: master
Choose a base branch
from
+74 −6
Draft

fix router response to user #1381

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 3 additions & 0 deletions src/Infrastructure/BotSharp.Abstraction/Routing/Models/RoutingArgs.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,9 @@ public class RoutingArgs
[JsonPropertyName("function")]
public string Function { get; set; } = "route_to_agent";

[JsonPropertyName("function_args")]
public string? FunctionArgs { get; set; }

/// <summary>
/// The reason why you select this function or agent
/// </summary>
Expand Down
23 changes: 21 additions & 2 deletions src/Infrastructure/BotSharp.Core/Routing/Reasoning/HFReasoner.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -51,9 +51,21 @@ public async Task<FunctionCallFromLlm> GetNextInstruction(Agent router, string m
}
};
var response = await completion.GetChatCompletions(router, dialogs);

var inst = response.Content.JsonContent<FunctionCallFromLlm>();

if (inst != null)
{
if (!string.IsNullOrEmpty(response.FunctionName))
{
inst.Function = response.FunctionName;
}

if (!string.IsNullOrEmpty(response.FunctionArgs))
{
inst.FunctionArgs = response.FunctionArgs;
}
}

// Fix LLM malformed response
await ReasonerHelper.FixMalformedResponse(_services, inst);

Expand All @@ -73,7 +85,14 @@ public async Task<bool> AgentExecuting(Agent router, FunctionCallFromLlm inst, R

// Set user content as Planner's question
message.FunctionName = inst.Function;
message.FunctionArgs = inst.Arguments == null ? "{}" : JsonSerializer.Serialize(inst.Arguments);
if (!string.IsNullOrEmpty(inst.FunctionArgs))
{
message.FunctionArgs = inst.FunctionArgs;
}
else
{
message.FunctionArgs = inst.Arguments == null ? "{}" : JsonSerializer.Serialize(inst.Arguments);
}
}

return true;
Expand Down
8 changes: 7 additions & 1 deletion src/Infrastructure/BotSharp.Core/Routing/Reasoning/InstructExecutor.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -31,6 +31,11 @@ await HookEmitter.Emit<IRoutingHook>(_services, async hook => await hook.OnRouti
}

message.FunctionArgs = JsonSerializer.Serialize(inst);
if (!string.IsNullOrEmpty(inst.FunctionArgs))
{
message.FunctionArgs = inst.FunctionArgs;
}
Comment on lines 33 to +37

@qodo-code-review qodo-code-review Bot Jul 16, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Action required

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


if (!string.IsNullOrEmpty(message.FunctionName))
{
var msg = RoleDialogModel.From(message, role: AgentRole.Function);
Expand All @@ -56,7 +61,7 @@ await HookEmitter.Emit<IRoutingHook>(_services, async hook => await hook.OnRouti
message = RoleDialogModel.From(message, role: AgentRole.Assistant, content: content);
dialogs.Add(message);
}
else
else if (!message.StopCompletion)
{
var state = _services.GetRequiredService<IConversationStateService>();
var useStreamMsg = state.GetState("use_stream_message");
Comment on lines +64 to 67

@qodo-code-review qodo-code-review Bot Jul 16, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Action required

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

Expand All @@ -71,6 +76,7 @@ await HookEmitter.Emit<IRoutingHook>(_services, async hook => await hook.OnRouti
var response = dialogs.Last();

response.Instruction = inst;
response.StopCompletion = message.StopCompletion;

@qodo-code-review qodo-code-review Bot Jul 16, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Action required

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


return response;
}
Expand Down
21 changes: 20 additions & 1 deletion src/Infrastructure/BotSharp.Core/Routing/Reasoning/NaiveReasoner.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -57,6 +57,18 @@ public async Task<FunctionCallFromLlm> GetNextInstruction(Agent router, string m
var response = await completion.GetChatCompletions(router, dialogs);

var inst = (response.FunctionArgs ?? response.Content).JsonContent<FunctionCallFromLlm>();
if (inst != null)
{
if (!string.IsNullOrEmpty(response.FunctionName))
{
inst.Function = response.FunctionName;
}

if (!string.IsNullOrEmpty(response.FunctionArgs))
{
inst.FunctionArgs = response.FunctionArgs;
}
}

// Fix LLM malformed response
await ReasonerHelper.FixMalformedResponse(_services, inst);
Expand All @@ -68,7 +80,14 @@ public Task<bool> AgentExecuting(Agent router, FunctionCallFromLlm inst, RoleDia
{
// Set user content as Planner's question
message.FunctionName = inst.Function;
message.FunctionArgs = inst.Arguments == null ? "{}" : JsonSerializer.Serialize(inst.Arguments);
if (!string.IsNullOrEmpty(inst.FunctionArgs))
{
message.FunctionArgs = inst.FunctionArgs;
}
else
{
message.FunctionArgs = inst.Arguments == null ? "{}" : JsonSerializer.Serialize(inst.Arguments);
}

return Task.FromResult(true);
}
Expand Down
25 changes: 23 additions & 2 deletions src/Infrastructure/BotSharp.Core/Routing/Reasoning/OneStepForwardReasoner.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -67,8 +67,22 @@ public async Task<FunctionCallFromLlm> GetNextInstruction(Agent router, string m
// eliminating format drift where the LLM completes with finishReason=stop and returns
// free text or JSON in Content instead of a structured function call.
var response = await GetChatCompletionsWithScopedState(completion, router, dialogs, "tool_choice", "required");

var inst = response.FunctionArgs?.JsonContent<FunctionCallFromLlm>();

if (inst != null)
{
if (!string.IsNullOrEmpty(response.FunctionName))
{
inst.Function = response.FunctionName;
}

if (!string.IsNullOrEmpty(response.FunctionArgs))
{
inst.FunctionArgs = response.FunctionArgs;
}
}


_logger.LogInformation("[OneStepForwardReasoner] ConversationId: {ConversationId}, MessageId: {MessageId}, Next instruction: {Instruction}",
_services.GetRequiredService<IRoutingContext>().ConversationId, messageId, response.FunctionArgs);

Expand All @@ -82,7 +96,14 @@ public Task<bool> AgentExecuting(Agent router, FunctionCallFromLlm inst, RoleDia
{
// Set user content as Planner's question
message.FunctionName = inst.Function;
message.FunctionArgs = inst.Arguments == null ? "{}" : JsonSerializer.Serialize(inst.Arguments);
if (!string.IsNullOrEmpty(inst.FunctionArgs))
{
message.FunctionArgs = inst.FunctionArgs;
}
else
{
message.FunctionArgs = inst.Arguments == null ? "{}" : JsonSerializer.Serialize(inst.Arguments);
}

return Task.FromResult(true);
}
Expand Down
Loading