add mcp http setting

[iceljc]

No description provided.

[qodo-code-review]

Review Summary by Qodo

Add MCP HTTP server configuration support

✨ Enhancement

Walkthroughs

Description

• Add HTTP transport configuration support for MCP servers
• Introduce McpHttpServerConfig class with shared base configuration
• Refactor McpSseServerConfig to inherit from McpHttpServerConfigBase
• Update McpClientManager to handle HTTP transport initialization
• Allow nullable environment variable values in stdio configuration

Diagram

flowchart LR
  A["McpServerConfigModel"] -->|"adds HttpConfig"| B["McpHttpServerConfig"]
  A -->|"refactors SseConfig"| C["McpSseServerConfig"]
  B -->|"inherits from"| D["McpHttpServerConfigBase"]
  C -->|"inherits from"| D
  E["McpClientManager"] -->|"checks HttpConfig first"| F["HttpClientTransport"]
  E -->|"falls back to SseConfig"| F

Loading

File Changes

1. src/Infrastructure/BotSharp.Abstraction/MCP/Models/McpServerConfigModel.cs ✨ Enhancement +11/-2

Introduce HTTP config and refactor base classes

• Add HttpConfig property to McpServerConfigModel for HTTP transport configuration
• Create new McpHttpServerConfig class inheriting from McpHttpServerConfigBase
• Extract common HTTP configuration into McpHttpServerConfigBase base class
• Refactor McpSseServerConfig to inherit from McpHttpServerConfigBase
• Change EnvironmentVariables type to allow nullable string values in McpStdioServerConfig

src/Infrastructure/BotSharp.Abstraction/MCP/Models/McpServerConfigModel.cs

---

2. src/Infrastructure/BotSharp.Core/MCP/Managers/McpClientManager.cs ✨ Enhancement +11/-1

Add HTTP transport initialization logic

• Add HTTP transport initialization logic in GetClient method
• Check HttpConfig before SseConfig for transport selection
• Create HttpClientTransport with HTTP-specific configuration options
• Maintain backward compatibility with existing SSE configuration

src/Infrastructure/BotSharp.Core/MCP/Managers/McpClientManager.cs

---

[qodo-code-review]

Code Review by Qodo

🐞 Bugs (2) 📘 Rule violations (1) 📎 Requirement gaps (0)

1. HttpConfig.EndPoint not validated 📘 Rule violation ☼ Reliability

Description

new Uri(config.HttpConfig.EndPoint) is executed without null/empty/format guards, so an invalid or
missing endpoint can throw and fall into the catch-all path rather than a clear boundary validation
fallback. This violates the requirement to add null/empty guards and safe fallbacks at provider
boundaries.

Code

src/Infrastructure/BotSharp.Core/MCP/Managers/McpClientManager.cs[36]

+                    Endpoint = new Uri(config.HttpConfig.EndPoint),

Evidence

PR Compliance ID 2 requires boundary-facing code to guard nullable/invalid inputs and return safe
fallbacks with clear logging. The new HTTP transport path constructs a Uri directly from
config.HttpConfig.EndPoint (which is declared with a null! default), with no
IsNullOrWhiteSpace/Uri.TryCreate validation before use.

src/Infrastructure/BotSharp.Core/MCP/Managers/McpClientManager.cs[36-36]
src/Infrastructure/BotSharp.Abstraction/MCP/Models/McpServerConfigModel.cs[45-45]
Best Practice: Learned patterns

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 HTTP MCP client creation path calls `new Uri(config.HttpConfig.EndPoint)` without validating for null/empty or invalid formats, which can throw and rely on the catch-all exception handler.

## Issue Context
`EndPoint` is declared with a `null!` initializer and may be missing/invalid when loaded from configuration. Compliance requires boundary-layer guards and a safe fallback (return `null`) with clear logging.

## Fix Focus Areas
- src/Infrastructure/BotSharp.Core/MCP/Managers/McpClientManager.cs[31-40]
- src/Infrastructure/BotSharp.Abstraction/MCP/Models/McpServerConfigModel.cs[43-48]

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

---

2. Ambiguous transport config 🐞 Bug ≡ Correctness

Description

McpServerConfigModel can now have both HttpConfig and SseConfig set, but GetMcpClientAsync silently
prioritizes HttpConfig when both are present. This makes a misconfiguration connect using an
unintended transport/endpoint with no warning, causing confusing connection failures.

Code

src/Infrastructure/BotSharp.Core/MCP/Managers/McpClientManager.cs[R31-44]

+            if (config.HttpConfig != null)
+            {
+                transport = new HttpClientTransport(new HttpClientTransportOptions
+                {
+                    Name = config.Name,
+                    Endpoint = new Uri(config.HttpConfig.EndPoint),
+                    AdditionalHeaders = config.HttpConfig.AdditionalHeaders,
+                    ConnectionTimeout = config.HttpConfig.ConnectionTimeout
+                });
+            }
+            else if (config.SseConfig != null)
            {
                transport = new HttpClientTransport(new HttpClientTransportOptions
                {

Evidence

The config model permits both transport sections at once, and the transport selection logic is an
if/else-if chain that always chooses HttpConfig first when both are non-null, with no validation or
logging for the ambiguous case.

src/Infrastructure/BotSharp.Abstraction/MCP/Models/McpServerConfigModel.cs[30-32]
src/Infrastructure/BotSharp.Core/MCP/Managers/McpClientManager.cs[30-50]

Agent prompt

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

### Issue description
`McpServerConfigModel` allows `HttpConfig`, `SseConfig`, and `StdioConfig` to be set simultaneously, but `McpClientManager.GetMcpClientAsync` silently prefers `HttpConfig` over `SseConfig` (and both over stdio). This makes configuration errors hard to detect.

### Issue Context
We should enforce or explicitly handle the invariant: exactly one transport config must be set for a server.

### Fix Focus Areas
- src/Infrastructure/BotSharp.Abstraction/MCP/Models/McpServerConfigModel.cs[30-32]
- src/Infrastructure/BotSharp.Core/MCP/Managers/McpClientManager.cs[30-66]

### Suggested fix
- Add a validation step (preferably in `GetMcpClientAsync` before constructing transports) that counts non-null configs among `HttpConfig`, `SseConfig`, `StdioConfig`.
- If count != 1, log a warning/error including `serverId` and which sections are set, then return null (or throw during startup, if that fits the app’s config philosophy).
- Optionally: document/encode an explicit precedence if you intentionally support multiple set, but still log that one was ignored.

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

---

3. Nullable env var values 🐞 Bug ☼ Reliability

Description

McpStdioServerConfig now allows null values in EnvironmentVariables, but the manager passes the
dictionary through to StdioClientTransportOptions without filtering/validation. Null values can
propagate to the stdio transport layer and trigger runtime failures or inconsistent environment
behavior.

Code

src/Infrastructure/BotSharp.Abstraction/MCP/Models/McpServerConfigModel.cs[54]

+    public Dictionary<string, string?>? EnvironmentVariables { get; set; }

Evidence

The PR changed the environment-variable dictionary value type from non-nullable to nullable, and the
only usage site passes it directly into the stdio transport options with no normalization step
(e.g., removing nulls).

src/Infrastructure/BotSharp.Abstraction/MCP/Models/McpServerConfigModel.cs[50-55]
src/Infrastructure/BotSharp.Core/MCP/Managers/McpClientManager.cs[51-60]

Agent prompt

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

### Issue description
`McpStdioServerConfig.EnvironmentVariables` now permits null values (`Dictionary<string, string?>?`). The stdio transport is constructed with this dictionary as-is, so null values can reach the transport implementation.

### Issue Context
Even if null is intended to mean “unset/remove”, passing null values through without defining semantics is unsafe; many environment-setting APIs expect non-null strings.

### Fix Focus Areas
- src/Infrastructure/BotSharp.Abstraction/MCP/Models/McpServerConfigModel.cs[50-55]
- src/Infrastructure/BotSharp.Core/MCP/Managers/McpClientManager.cs[51-60]

### Suggested fix options
1) If null values are not intended:
- Change back to `Dictionary<string, string>?` and treat missing keys as absence.

2) If null values are intended to mean “remove/unset”:
- In `GetMcpClientAsync`, before assigning to `StdioClientTransportOptions.EnvironmentVariables`, create a new dictionary containing only entries where value != null.
- Optionally log a warning if any null-valued entries were provided (to surface config mistakes).

Implement one approach consistently and add a small comment clarifying the semantics.

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

---

ⓘ The new review experience is currently in Beta. Learn more