Skip to content

Conversation

iceljc
Copy link
Collaborator

@iceljc iceljc commented Aug 6, 2025

PR Type

Enhancement


Description

  • Serialize data to JSON before sending chat events

  • Remove ExcludedFunctions configuration and related filtering logic

  • Clean up commented code in hook classes


Diagram Walkthrough

flowchart LR
  A["Chat Event Data"] --> B["JSON Serialization"]
  B --> C["EventEmitter.SendChatEvent"]
  D["PluginSettings"] --> E["Remove ExcludedFunctions"]
  E --> F["Simplified Plugin Loading"]
Loading

File Walkthrough

Relevant files
Configuration changes
PluginLoaderSettings.cs
Remove ExcludedFunctions configuration property                   

src/Infrastructure/BotSharp.Abstraction/Plugins/PluginLoaderSettings.cs

  • Remove ExcludedFunctions property from PluginSettings class
+0/-2     
appsettings.json
Remove ExcludedFunctions from configuration                           

src/WebStarter/appsettings.json

  • Remove ExcludedFunctions configuration array containing
    "McpToolAdapter"
+0/-3     
Enhancement
BotSharpCoreExtensions.cs
Simplify plugin function registration logic                           

src/Infrastructure/BotSharp.Core/BotSharpCoreExtensions.cs

  • Remove excludedFunctions variable and related filtering logic
  • Simplify function registration to load all IFunctionCallback
    implementations
+1/-3     
ChatHubConversationHook.cs
Add JSON serialization for chat events                                     

src/Plugins/BotSharp.Plugin.ChatHub/Hooks/ChatHubConversationHook.cs

  • Serialize data to JSON before calling SendChatEvent
  • Remove commented InitClientConversation call
+2/-2     
ChatHubCrontabHook.cs
Add JSON serialization for crontab notifications                 

src/Plugins/BotSharp.Plugin.ChatHub/Hooks/ChatHubCrontabHook.cs

  • Serialize notification data to JSON before sending to SignalR clients
+2/-1     
StreamingLogHook.cs
Add JSON serialization and cleanup comments                           

src/Plugins/BotSharp.Plugin.ChatHub/Hooks/StreamingLogHook.cs

  • Serialize data to JSON before calling SendChatEvent
  • Remove multiple commented SendContentLog calls
+2/-8     
WelcomeHook.cs
Add JSON serialization for welcome events                               

src/Plugins/BotSharp.Plugin.ChatHub/Hooks/WelcomeHook.cs

  • Serialize data to JSON before calling SendChatEvent
+2/-1     
ChatHubObserver.cs
Add JSON serialization and options dependency                       

src/Plugins/BotSharp.Plugin.ChatHub/Observers/ChatHubObserver.cs

  • Add BotSharpOptions dependency injection
  • Serialize data to JSON before calling SendChatEvent
+6/-2     

@iceljc iceljc merged commit 60ecfea into SciSharp:master Aug 6, 2025
0 of 3 checks passed
Copy link

qodo-merge-pro bot commented Aug 6, 2025

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

Logic Change

The removal of ExcludedFunctions filtering logic means all IFunctionCallback implementations will now be registered. This could potentially load previously excluded functions that were intentionally disabled, which may cause unexpected behavior or security issues.

var functions = assembly.GetTypes()
    .Where(x => x.IsClass
            && x.GetInterface(nameof(IFunctionCallback)) != null)
Error Handling

The SendEvent method has an empty try-catch block that silently swallows all exceptions. This makes debugging difficult and could hide important errors during JSON serialization or SignalR communication.

    try
    {
        var json = JsonSerializer.Serialize(data, _options.JsonSerializerOptions);
        await _chatHub.Clients.User(item.UserId).SendAsync(ChatEvent.OnNotificationGenerated, json);
    }
    catch { }
}
Missing Import

The code uses JsonSerializer.Serialize but there's no visible using statement for System.Text.Json. This could cause compilation errors if the import is not present elsewhere in the file.

var json = JsonSerializer.Serialize(data, _options.JsonSerializerOptions);
EventEmitter.SendChatEvent(_services, _logger, @event, conversationId, user?.Id, json, nameof(ChatHubObserver), callerName)

Copy link

qodo-merge-pro bot commented Aug 6, 2025

PR Code Suggestions ✨

Explore these optional code suggestions:

CategorySuggestion                                                                                                                                    Impact
General
Handle serialization exceptions gracefully

Wrap the JSON serialization in a try-catch block to handle potential
serialization exceptions. The synchronous blocking of async operations using
GetAwaiter().GetResult() can cause deadlocks in certain contexts.

src/Plugins/BotSharp.Plugin.ChatHub/Observers/ChatHubObserver.cs [139-141]

-var json = JsonSerializer.Serialize(data, _options.JsonSerializerOptions);
-EventEmitter.SendChatEvent(_services, _logger, @event, conversationId, user?.Id, json, nameof(ChatHubObserver), callerName)
-             .ConfigureAwait(false).GetAwaiter().GetResult();
+try
+{
+    var json = JsonSerializer.Serialize(data, _options.JsonSerializerOptions);
+    EventEmitter.SendChatEvent(_services, _logger, @event, conversationId, user?.Id, json, nameof(ChatHubObserver), callerName)
+                 .ConfigureAwait(false).GetAwaiter().GetResult();
+}
+catch (JsonException ex)
+{
+    _logger.LogError(ex, "Failed to serialize data for event {Event}", @event);
+}
  • Apply / Chat
Suggestion importance[1-10]: 6

__

Why: The suggestion correctly identifies that JSON serialization can fail and proposes a try-catch block with logging, which improves the robustness and observability of the error handling.

Low
Add null check before serialization

Add null check for data parameter before serialization to prevent potential null
reference exceptions. Consider handling serialization failures with try-catch
block to ensure robust error handling.

src/Plugins/BotSharp.Plugin.ChatHub/Hooks/ChatHubConversationHook.cs [174-175]

+if (data == null) return;
 var json = JsonSerializer.Serialize(data, _options.JsonSerializerOptions);
 await EventEmitter.SendChatEvent(_services, _logger, @event, conversationId, user?.Id, json, nameof(ChatHubConversationHook), callerName);
  • Apply / Chat
Suggestion importance[1-10]: 4

__

Why: The suggestion's reasoning is incorrect as JsonSerializer.Serialize handles nulls without throwing an exception, but adding a null check is a reasonable defensive coding practice to avoid sending events with a "null" payload.

Low
  • More

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant