Skip to content
Merged
Changes from all commits
Commits
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
103 changes: 101 additions & 2 deletions src/Infrastructure/BotSharp.Core/Routing/Executor/MCPToolExecutor.cs
Original file line number Diff line number Diff line change
@@ -1,5 +1,7 @@
using BotSharp.Abstraction.Routing.Executor;
using BotSharp.Core.MCP.Managers;
using BotSharp.Core.MessageHub;
using ModelContextProtocol;
using ModelContextProtocol.Protocol;

namespace BotSharp.Core.Routing.Executor;
Expand Down Expand Up @@ -33,8 +35,12 @@ public async Task<bool> ExecuteAsync(RoleDialogModel message)
return false;
}

// Call the tool through mcpdotnet
var result = await client.CallToolAsync(_functionName, !argDict.IsNullOrEmpty() ? argDict : []);
// Call the tool through mcpdotnet, relaying whatever progress it reports along the
// way — see ToolProgressIndicator for why the reporter has to be supplied here.
var result = await client.CallToolAsync(
_functionName,
!argDict.IsNullOrEmpty() ? argDict : [],
progress: ToolProgressIndicator.For(_services, message));

// Extract the text content from the result
var json = string.Join("\n", result.Content.Where(c => c is TextContentBlock).Select(c => ((TextContentBlock)c).Text));
Expand All @@ -55,6 +61,99 @@ public Task<string> GetIndicatorAsync(RoleDialogModel message)
return Task.FromResult(message.Indication ?? string.Empty);
}

/// <summary>
/// Turns a tool's <c>notifications/progress</c> into indications, so a long call can say what
/// it is doing while it does it.
/// <para>
/// WHY THIS EXISTS. RoutingService pushes one indication when a function starts, and for a
/// tool that returns in a second that is the whole story. An MCP tool driving a browser takes
/// minutes, and that single line — "Working out the steps" — was all the chat had to show for
/// the entire run: no way to tell a task making progress from one that had wedged.
/// </para>
/// <para>
/// Supplying the reporter is also what MAKES a server report. The SDK only attaches a
/// <c>progressToken</c> to the request when this is non-null, and a server with no token to
/// answer has nowhere to send notifications — computer-autoplay's <c>await_web_task</c>
/// returns early from its own reporter for exactly that reason. So the argument is not an
/// optimisation of an existing stream; it is what opens it.
/// </para>
/// <para>
/// Nothing is required of a server that does not report progress: no notifications arrive,
/// <see cref="Report"/> is never called, and the call behaves as it did before.
/// </para>
/// </summary>
private sealed class ToolProgressIndicator : IProgress<ProgressNotificationValue>
{
private readonly MessageHub<HubObserveData<RoleDialogModel>> _hub;
private readonly string _conversationId;
private readonly RoleDialogModel _message;

private ToolProgressIndicator(
MessageHub<HubObserveData<RoleDialogModel>> hub,
string conversationId,
RoleDialogModel message)
{
_hub = hub;
_conversationId = conversationId;
_message = message;
}

/// <summary>
/// A reporter for this call, or null when there is no conversation to report into — a
/// tool invoked outside one, from a task or a test. Null is the right answer there rather
/// than a reporter that drops everything: it also tells the server not to bother sending.
/// <para>
/// The conversation id is read HERE, on the thread that starts the call, and captured.
/// <see cref="Report"/> runs on whichever thread the MCP transport is reading on, and
/// resolving a scoped service from there to ask again would be a race for a value that
/// cannot change during the call.
/// </para>
/// </summary>
public static ToolProgressIndicator? For(IServiceProvider services, RoleDialogModel message)
{
var conversationId = services.GetRequiredService<IConversationService>().ConversationId;
if (string.IsNullOrWhiteSpace(conversationId))
{
return null;
}

var hub = services.GetRequiredService<MessageHub<HubObserveData<RoleDialogModel>>>();
return new ToolProgressIndicator(hub, conversationId, message);
}

/// <summary>
/// Pushes one indication per reported step.
/// <para>
/// Deliberately synchronous, which is why this is a hand-written IProgress rather than a
/// <see cref="Progress{T}"/>: Progress&lt;T&gt; queues every report to the thread pool
/// independently, so two steps reported together can arrive out of order — and the one
/// that arrives last is the one left on screen. Pushing inline keeps the order the server
Comment on lines +127 to +130

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.

Remediation recommended

2. Progress blocks tool io 🐞 Bug ➹ Performance

ToolProgressIndicator.Report is deliberately synchronous and triggers the full indication delivery
path inline, so frequent progress notifications can block the MCP notification/transport thread and
delay tool completion/response processing. In the SSE path, each indication can synchronously wait
on network I/O and an explicit Task.Delay(10), amplifying latency under many progress steps or slow
clients.
Agent Prompt
### Issue description
The progress callback runs synchronously and pushes indications through `MessageHub`, which invokes observers inline. Downstream observers synchronously wait for async listeners (including SSE writes), so the MCP progress callback thread can be blocked by network I/O and artificial delays.

### Issue Context
The implementation intentionally avoided `Progress<T>` to preserve ordering. You can preserve ordering without blocking the MCP callback thread by using a single-threaded ordered queue/Channel consumer.

### Fix Focus Areas
- src/Infrastructure/BotSharp.Core/Routing/Executor/MCPToolExecutor.cs[124-154]

### What to change
- Replace direct `_hub.Push(...)` inside `Report` with an ordered async dispatcher:
  - Use `Channel<ProgressNotificationValue>` (bounded) or an `ActionBlock`/queue.
  - `Report(...)` should enqueue quickly (non-blocking) and return.
  - A single background consumer (per tool call) should read in order and push to the hub.
- Consider coalescing/throttling repeated messages to reduce UI spam.
- Keep best-effort semantics (drop on backpressure rather than blocking the transport thread).

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

/// sent them in.
/// </para>
/// </summary>
public void Report(ProgressNotificationValue value)
{
// A bare count with no message is a progress BAR's input, not a sentence, and the
// chat has nowhere to put it. Servers that only send numbers are simply not relayed.
if (string.IsNullOrWhiteSpace(value.Message))
{
return;
}

// Cloned: this is pushed to observers that read it, and the function's own message is
// still being used by the call in flight. Its indication is not ours to overwrite.
var indication = RoleDialogModel.From(_message);
indication.Indication = value.Message;

_hub.Push(new()
{
EventName = ChatEvent.OnIndicationReceived,
Data = indication,
Comment on lines +148 to +151

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. Observer exceptions break tool 🐞 Bug ☼ Reliability

ToolProgressIndicator.Report pushes progress into MessageHub without guarding against
subscriber/listener exceptions; failures in the indication pipeline (e.g., SSE writes when a client
disconnects) can propagate back into the MCP progress callback and cause CallToolAsync to
fault/abort. This makes tool execution reliability depend on UI/observer health rather than being
best-effort telemetry.
Agent Prompt
### Issue description
`ToolProgressIndicator.Report` calls `_hub.Push(...)` inline. Because `MessageHub.Push` directly calls `OnNext` on a synchronized `Subject`, any exception thrown by observers/listeners can bubble back through `Push` into the MCP SDK's progress callback, potentially failing the tool call.

### Issue Context
Indication listeners include SSE handlers that write to `HttpResponse.Body`. Those writes can throw (e.g., client disconnect), and `ConversationObserver` executes listeners synchronously (`GetResult()`), so the exception can propagate to the progress callback thread.

### Fix Focus Areas
- src/Infrastructure/BotSharp.Core/Routing/Executor/MCPToolExecutor.cs[134-154]

### What to change
- Wrap `_hub.Push(...)` in a `try/catch` inside `Report`.
- On exception, swallow (best-effort) and log via an injected `ILogger` (either pass one into `ToolProgressIndicator` via `For(...)`, or resolve it in `For(...)` and store it).
- Ensure the tool execution result is not impacted by notification delivery failures.

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

RefId = _conversationId
});
}
}


private static Dictionary<string, object?> JsonToDictionary(string? json)
{
Expand Down
Loading