Add ToolProgressIndicator for real-time tool progress updates

[hchen2020]

No description provided.

[qodo-code-review]

PR Summary by Qodo

Relay MCP tool progress to chat via ToolProgressIndicator

✨ Enhancement 🕐 10-20 Minutes

AI Description

• Attach an MCP progress reporter to tool calls to enable server-side progress streaming.
• Convert MCP progress notifications into chat indications via MessageHub events.
• Avoid progress reporting outside conversations and preserve message ordering/thread-safety.

Diagram

graph TD
  Exec["McpToolExecutor"] --> Client["MCP Client"] --> Tool["MCP Tool Server"]
  Tool -- "notifications/progress" --> Ind["ToolProgressIndicator"] --> Hub["MessageHub"] --> UI["Chat/SSE clients"]
  Exec -- "pass IProgress" --> Ind

  subgraph Legend
    direction LR
    _comp["Component"] ~~~ _svc["Service"] ~~~ _ext{{"External"}}
  end

Loading

High-Level Assessment

The following are alternative approaches to this PR:

1. Throttle/aggregate progress indications

• ➕ Prevents high-frequency progress spam from overwhelming observers/clients
• ➕ Reduces UI flicker and event volume on very chatty tools
• ➖ Adds buffering complexity and introduces latency; can obscure fine-grained progress steps
• ➖ Requires choosing throttle policy (time/window/last-value) and tuning

2. Introduce a dedicated ToolProgress event instead of reusing indications

• ➕ Separates 'status text' from 'tool progress', enabling richer UI (percent bars, step lists)
• ➕ Avoids overloading indication semantics for non-tool contexts
• ➖ Requires changes across observers/controllers/clients to handle a new event type
• ➖ Bigger surface area than this PR’s minimal, backward-compatible approach

Recommendation: The current approach is the right minimal integration: it leverages the existing OnIndicationReceived pipeline and only activates server progress streaming when a conversation exists (by passing a non-null reporter). Consider adding optional throttling later if real-world tools emit progress at very high frequency, but keep the default behavior unthrottled to preserve step fidelity and ordering.

Files changed (1) +101 / -2

Enhancement (1) +101 / -2

MCPToolExecutor.csAdd MCP progress-to-indication relay via ToolProgressIndicator +101/-2

Add MCP progress-to-indication relay via ToolProgressIndicator

• Passes a progress reporter into MCP CallToolAsync so servers can emit notifications/progress during long-running tool calls. Adds a nested ToolProgressIndicator that captures conversation context, clones the current RoleDialogModel, and pushes OnIndicationReceived updates through MessageHub while preserving report order and avoiding reporting when no conversation is active.

src/Infrastructure/BotSharp.Core/Routing/Executor/MCPToolExecutor.cs

[qodo-code-review]

Code Review by Qodo

🐞 Bugs (2) 📘 Rule violations (0) 📜 Skill insights (0)

1. Observer exceptions break tool 🐞 Bug ☼ Reliability

Description

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.

Code

src/Infrastructure/BotSharp.Core/Routing/Executor/MCPToolExecutor.cs[R148-151]

+            _hub.Push(new()
+            {
+                EventName = ChatEvent.OnIndicationReceived,
+                Data = indication,

Evidence

The new progress bridge calls MessageHub synchronously and does not catch exceptions. MessageHub
propagates subscriber exceptions to the caller, and the indication observer runs listeners
synchronously; the SSE listener writes to the response stream and can throw, which would then bubble
back into the progress callback path.

src/Infrastructure/BotSharp.Core/Routing/Executor/MCPToolExecutor.cs[134-154]
src/Infrastructure/BotSharp.Core/MessageHub/MessageHub.cs[16-23]
src/Infrastructure/BotSharp.Core/MessageHub/Observers/ConversationObserver.cs[30-45]
src/Infrastructure/BotSharp.OpenAPI/Controllers/Conversation/ConversationController.cs[450-453]
src/Infrastructure/BotSharp.OpenAPI/Controllers/Conversation/ConversationController.cs[553-563]

Agent prompt

The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

### 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

---

2. Progress blocks tool IO 🐞 Bug ➹ Performance

Description

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.

Code

src/Infrastructure/BotSharp.Core/Routing/Executor/MCPToolExecutor.cs[R127-130]

+        /// 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

Evidence

The new code explicitly keeps progress reporting synchronous and pushes into MessageHub, which calls
subscribers inline. The indication observer executes listeners synchronously, and the SSE listener
performs awaited writes and an extra delay, meaning progress delivery can block the thread that
invoked Report.

src/Infrastructure/BotSharp.Core/Routing/Executor/MCPToolExecutor.cs[124-154]
src/Infrastructure/BotSharp.Core/MessageHub/MessageHub.cs[16-23]
src/Infrastructure/BotSharp.Core/MessageHub/Observers/ConversationObserver.cs[39-44]
src/Infrastructure/BotSharp.OpenAPI/Controllers/Conversation/ConversationController.cs[553-563]

Agent prompt

The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

### 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

---

_{To customize comments, go to the Qodo configuration screen, or learn more in the docs.}

[qodo-code-review]

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

[qodo-code-review]

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