refine init mcp

[iceljc]

PR Type

Enhancement, Bug fix

---

Description

• Added error handling for RegisterFunctionCall and Execute methods.

• Improved robustness in McpToolAdapter with try-catch blocks.

• Fixed potential null or empty dictionary issues in JSON parsing.

• Enhanced logging for tool execution errors in McpToolAdapter.

---

Changes walkthrough 📝

Relevant files

Error handling

BotSharpMCPExtensions.cs Added error handling in `RegisterFunctionCall` method        src/Infrastructure/BotSharp.Core.MCP/BotSharpMCPExtensions.cs Added try-catch block to RegisterFunctionCall. Prevented unhandled exceptions during tool registration. Improved service registration logic for tools and callbacks. +13/-9    McpToolAdapter.cs Improved error handling and robustness in `McpToolAdapter` src/Infrastructure/BotSharp.Core.MCP/Functions/McpToolAdapter.cs Wrapped Execute method logic in a try-catch block. Added error message handling for tool execution failures. Fixed potential issues with empty or null JSON arguments. Enhanced robustness in tool execution and result processing. +24/-14

---

Need help?

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

Check out the documentation for more information.

[qodo-merge-pro]

Qodo Merge was enabled for this repository. To continue using it, please link your Git account with your Qodo account here.

PR Reviewer Guide 🔍

Here are some key observations to aid the review process:

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

🧪 No relevant tests

🔒 No security concerns identified

⚡ Recommended focus areas for review Empty Catch Block The empty catch block in RegisterFunctionCall silently swallows all exceptions without logging or handling them, which could hide critical initialization errors. catch { } } Potential NullReferenceException The code assumes agent.McpTools.Where(...).FirstOrDefault() will return a non-null value. If no matching tool is found, this could lead to a NullReferenceException when accessing ServerId. var serverId = agent.McpTools.Where(t => t.Functions.Any(f => f.Name == Name)).FirstOrDefault().ServerId;

[GGHansome]

Auto Review Result:

Code Review Summary

Change Summary: The code changes primarily focus on adding error handling to two distinct functions. In the RegisterFunctionCall method, a try-catch block has been introduced around the logic to ensure that any exceptions are silently ignored, which could prevent a service from crashing during runtime. Similarly, in the Execute method of the McpToolAdapter class, a try-catch block has been implemented, and errors are now returned as part of the message content in case of exceptions. These changes improve system robustness by preventing crashes but might hide underlying issues requiring attention.

Issues Found

Issue 1: Inadequate Exception Handling

• Description: Catching all exceptions without any logging or specific handling could suppress critical error information, making debugging difficult.
• Suggestion: Implement logging within the catch block to at least capture and record the exceptions for future diagnosis.
• Example:

  catch (Exception ex)
  {
      // Log exception details
      Console.WriteLine($"An error occurred: {ex.Message}");
  }

Issue 2: Use of Non-Descriptive Catch Block in RegisterFunctionCall

• Description: The catch block in the RegisterFunctionCall is specified without any exception type (catch { }), which can lead to catching unintended exceptions.
• Suggestion: Specify an exception type to catch or handle potential exceptions more robustly, such as catch (Exception ex) {} with handling logic.
• Example:

  catch (Exception ex)
  {
      // Handle or log exception
      Console.WriteLine($"An error occurred in RegisterFunctionCall: {ex.Message}");
  }

Issue 3: Use of Array Initialization in JsonToDictionary

• Description: The method JsonToDictionary attempts to return an empty array [] in a context where it seems a dictionary is expected.
• Suggestion: Return an empty dictionary instead of [].
• Example:

  return new Dictionary<string, object>();

Overall Evaluation

The addition of try-catch blocks improves the system’s ability to handle runtime errors without crashing. However, these error handling mechanisms should be supplemented by implementing logging to ensure that errors are recorded and can be reviewed. Also, ensure that exception handling is correctly targeting the specific issues for efficient debugging. Additionally, check the types returned in certain methods, such as JsonToDictionary, to prevent type mismatch errors.

[qodo-merge-pro]

Qodo Merge was enabled for this repository. To continue using it, please link your Git account with your Qodo account here.

PR Code Suggestions ✨

Explore these optional code suggestions:

Category	Suggestion	Impact
Possible issue	Fix inverted condition The negation operator ! before argDict.IsNullOrEmpty() inverts the condition, causing empty dictionaries to be used instead of the empty array. This is likely the opposite of what was intended. src/Infrastructure/BotSharp.Core.MCP/Functions/McpToolAdapter.cs [43] -var result = await client.CallToolAsync(_tool.Name, !argDict.IsNullOrEmpty() ? argDict : []); +var result = await client.CallToolAsync(_tool.Name, argDict.IsNullOrEmpty() ? [] : argDict); Apply this suggestion Suggestion importance[1-10]: 10 __ Why: This suggestion fixes a critical logical error where the condition is inverted, causing empty dictionaries to be used when they shouldn't be. This could lead to incorrect behavior in tool execution.	High
	Handle potential null reference The code assumes that FirstOrDefault() will always return a non-null object and directly accesses its ServerId property. This could cause a NullReferenceException if no matching tool is found. src/Infrastructure/BotSharp.Core.MCP/Functions/McpToolAdapter.cs [38] -var serverId = agent.McpTools.Where(t => t.Functions.Any(f => f.Name == Name)).FirstOrDefault().ServerId; +var tool = agent.McpTools.FirstOrDefault(t => t.Functions.Any(f => f.Name == Name)); +if (tool == null) +{ + throw new InvalidOperationException($"No tool configuration found for function '{Name}'"); +} +var serverId = tool.ServerId; Apply this suggestion Suggestion importance[1-10]: 9 __ Why: This suggestion addresses a critical potential NullReferenceException that could occur if no matching tool is found. The improved code properly checks for null before accessing the ServerId property.	High
	Add exception logging The empty catch block silently swallows all exceptions during tool registration, which could hide critical initialization errors. Add logging or at least handle specific exceptions to make troubleshooting easier. src/Infrastructure/BotSharp.Core.MCP/BotSharpMCPExtensions.cs [62] -catch { } +catch (Exception ex) +{ + // Log the exception or handle specific cases + Console.WriteLine($"Failed to register MCP tools: {ex.Message}"); +} Apply this suggestion Suggestion importance[1-10]: 8 __ Why: The empty catch block silently suppresses all exceptions, which could hide critical initialization errors. Adding proper exception logging would significantly improve troubleshooting and debugging capabilities.	Medium
More