Refactor InvokeFunction code

[geffzhang]

User description

Refactor the InvokeFunction code by categorizing the dummy function, function callback, and MCP tool invocation logic into three distinct classes

---

PR Type

Enhancement

---

Description

• Refactored function invocation logic into executor classes

  • Introduced IFunctionExecutor interface and multiple implementations
  • Centralized function execution via FunctionExecutorFactory

• Migrated and improved MCP integration

  • Moved MCP service registration and helpers to core infrastructure
  • Enhanced MCP client management and tool loading

• Removed obsolete MCP project and related files

  • Deleted BotSharp.Core.MCP project and references
  • Updated solution and project files accordingly

• Upgraded ModelContextProtocol dependency to 0.1.0-preview.11

---

Changes walkthrough 📝

	Relevant files
Enhancement	16 files IFunctionExecutor.cs Add IFunctionExecutor interface for function execution      +15/-0    BotSharpMCPExtensions.cs Remove MCP service registration from old location                +0/-64    Using.cs Remove global usings for obsolete MCP project                        +0/-18    BotSharpMCPExtensions.cs Add MCP service registration to core infrastructure            +35/-0    AiFunctionHelper.cs Add helper for mapping MCP tools to FunctionDef                    [link]    MCPToolAgentHook.cs Add agent hook for loading MCP tools as functions                [link]    McpClientManager.cs Add MCP client manager for server connections                        [link]    McpService.cs Update MCP service to use new client manager                          +9/-4      MCPSettings.cs Add MCP settings model to core infrastructure                        [link]    DummyFunctionExecutor.cs Add executor for dummy function output                                      +37/-0    FunctionCallbackExecutor.cs Add executor for function callbacks                                            +23/-0    FunctionExecutorFactory.cs Add factory for creating function executors                            +41/-0    IFunctionExecutor.cs Add IFunctionExecutor interface to executor namespace        +8/-0      MCPFunctionExecutor.cs Add executor for MCP tool function calls                                  +21/-27  RoutingService.InvokeFunction.cs Refactor function invocation to use executors                        +16/-72  Using.cs Update global usings for new structure                                      +37/-36
Configuration changes	3 files BotSharp.sln Remove obsolete MCP project from solution                                +0/-11    BotSharp.Core.MCP.csproj Remove obsolete MCP project file                                                  +0/-18    WebStarter.csproj Remove reference to obsolete MCP project                                  +0/-1
Dependencies	2 files Directory.Packages.props Upgrade ModelContextProtocol package version                          +2/-2      BotSharp.Core.csproj Add ModelContextProtocol package to core project                  +1/-0

---

Need help?

Type /help how to ... in the comments thread for any questions about Qodo Merge usage.

Check out the documentation for more information.

[GGHansome]

Auto Review Result:

Code Review Summary

Change Overview: The main goal of the code changes appears to be the restructuring of the function execution logic within the BotSharp infrastructure. The changes introduce a new contract for executing functions (IFunctionExecutor), along with specific implementations for executing dummy functions and callbacks related to the MCP (Model Context Protocol). This refactor aims to standardize function execution and improve the modularity of the code.

Issues Identified

Issue 1: Code Duplication

• Description: There are instances of code duplication, particularly around handling function execution logic (e.g., determining which function executor to use based on conditions).
• Suggestion: Consider implementing a common handler or strategy pattern to manage different execution flows without duplicating the same logic in multiple places.
• Example:

  // Similar logic in multiple classes
  if (functioncall != null)
  {
      return new FunctionCallbackExecutor(functioncall);
  }
  // This can be refactored to a single method in a factory or strategy class.

Issue 2: Asynchronous Code Handling

• Description: The use of .Result on asynchronous operations can lead to deadlocks and blocking of threads, especially in UI contexts.
• Suggestion: Replace synchronous waiting with async/await pattern to avoid potential blocking issues.
• Example:

  // Current
  var tools = _mcpClientManager.GetMcpClientAsync(config.Id).Result.ListToolsAsync().Result;
  // Suggested
  var tools = await (await _mcpClientManager.GetMcpClientAsync(config.Id)).ListToolsAsync();

Issue 3: Lack of Error Handling

• Description: Some methods suppress exceptions by using an empty catch block, which can hide errors from being caught and logged properly.
• Suggestion: Implement logging and/or rethrow exceptions where applicable to ensure issues are handled or propagated appropriately.
• Example:

  try
  {
      // some code
  }
  catch (Exception ex)
  {
      // Log exception
      _logger.LogError(ex, "Error executing function");
      throw;
  }

Overall Evaluation

The code changes introduce a thoughtful reorganization by abstracting the function execution into a more modular design. The use of interfaces such as IFunctionExecutor enhances the extensibility and testability of the system. However, the codebase could benefit from a more consistent error handling approach and reducing duplicated logic, particularly in areas dealing with asynchronous executions. Refactoring patterns like Dependency Injection are well-utilized, promoting better separation of concerns.

[qodo-merge-pro]

PR Reviewer Guide 🔍

Here are some key observations to aid the review process:

⏱️ Estimated effort to review: 3 🔵🔵🔵⚪⚪

🧪 No relevant tests

🔒 No security concerns identified

⚡ Recommended focus areas for review Method Mismatch The interface implementation methods have different names than the interface definition. The class implements ExecuteAsync and GetIndicatorAsync but the interface defines Execute and GetIndication. public async Task<bool> ExecuteAsync(RoleDialogModel message) { var render = _services.GetRequiredService<ITemplateRender>(); var state = _services.GetRequiredService<IConversationStateService>(); var dict = new Dictionary<string, object>(); foreach (var item in state.GetStates()) { dict[item.Key] = item.Value; } var text = render.Render(functionDef.Output, dict); message.Content = text; return true; } public async Task<string> GetIndicatorAsync(RoleDialogModel message) { return "Running"; } Method Mismatch The interface implementation methods have different names than the interface definition. The class implements ExecuteAsync and GetIndicatorAsync but the interface defines Execute and GetIndication. public async Task<bool> ExecuteAsync(RoleDialogModel message) { return await functionCallback.Execute(message); } public async Task<string> GetIndicatorAsync(RoleDialogModel message) { return await functionCallback.GetIndication(message); } Potential Null Reference The code calls ExecuteAsync on funcExecutor without checking if it's null after the factory creation. While there is a null check, it only handles the case when funcExecutor is null, but doesn't ensure it's not null before calling methods on it. result = await funcExecutor.ExecuteAsync(clonedMessage);

[qodo-merge-pro]

PR Code Suggestions ✨

No code suggestions found for the PR.

[GGHansome]

Auto Review Result:

Code Review Summary

Change Summary: The code changes primarily involve the refactoring of function calls within the BotSharp framework to implement the IFunctionExecutor interface, promoting better separation of concerns and maintainability by abstracting function execution logic into dedicated executor classes. These changes impact how functions are invoked and managed, facilitating easier future extensions and potential optimizations.

Identified Issues

Issue 1: Redundant Async/Await Usage

• Description: The function ExecuteAsync in DummyFunctionExecutor, FunctionCallbackExecutor, and MCPToolExecutor uses async/await on tasks that do not require await if they are not using async operations internally.
• Suggestion: Remove async/await from tasks that do not perform asynchronous operations to reduce unnecessary overhead.
• Example:

  // Before
  public async Task<bool> ExecuteAsync(RoleDialogModel message)
  {
      // Perform async operation
  }
  
  // After
  public Task<bool> ExecuteAsync(RoleDialogModel message)
  {
      // Directly return Task if no async operation
  }
  

Issue 2: Missing Exception Handling

• Description: In the RegisterFunctionCall method, exceptions are silently caught without logging or handling which can make debugging difficult.
• Suggestion: Add logging or rethrow the exception with additional context to understand why an error occurred.
• Example:

  catch (Exception ex)
  {
      // Log exception
      _logger.LogError(ex, "An error occurred while registering function call.");
  }

Issue 3: Hard-coded Values

• Description: The code has hard-coded strings like "Running" in GetIndicatorAsync which reduces flexibility and can lead to issues if messages need to be localized.
• Suggestion: Use constants or resource files for such values.
• Example:

  private const string RunningIndicator = "Running";
  
  return RunningIndicator;

Overall Assessment

The code demonstrates a good effort toward better modularization by introducing executor interfaces and classes. The primary focus should be on improving the efficiency of asynchronous operations, adding robust exception handling, and avoiding hard-coded strings for better maintainability and localization support.

[iceljc]

I think it is not necessary to pass "functionCallback" from outside. Just function name will be enough.

[geffzhang]

functionCallback is botsharp functionexecutor：FunctionCallbackExecutor，Determine whether it is a FunctionCalling defined within the BotSharp framework by checking if FunctionCallback is null