Skip to content

Conversation

@iceljc
Copy link
Collaborator

@iceljc iceljc commented Oct 20, 2025

PR Type

Enhancement, Bug fix


Description

  • Replace Dictionary<string, object> with IDictionary<string, object> and ConcurrentDictionary for thread-safe concurrent access

  • Refactor DeepClone method to support object modification and use centralized BotSharpOptions

  • Add ReferenceHandler.IgnoreCycles to JSON serialization options to prevent circular reference errors

  • Replace .Result calls with .ConfigureAwait(false).GetAwaiter().GetResult() for async safety

  • Add Quality property to ImageEditSetting and improve image editing functionality

  • Remove unused ChatHubCrontabHook class and clean up unnecessary imports


Diagram Walkthrough

flowchart LR
  A["Dictionary to IDictionary<br/>ConcurrentDictionary"] -->|Thread-safe| B["Rendering Methods"]
  C["DeepClone Enhancement<br/>with Modifier"] -->|Flexible| D["Object Cloning"]
  E["ReferenceHandler<br/>IgnoreCycles"] -->|Prevent Cycles| F["JSON Serialization"]
  G["Async Safety<br/>ConfigureAwait"] -->|Proper Async| H["Service Calls"]
  I["ImageEditSetting<br/>Quality Property"] -->|Enhanced| J["Image Operations"]
Loading

File Walkthrough

Relevant files
Enhancement
14 files
IAgentService.cs
Replace Dictionary with IDictionary and ConcurrentDictionary
+8/-7     
LlmModelSetting.cs
Add Quality property to ImageEditSetting                                 
+1/-0     
ITemplateRender.cs
Replace Dictionary with IDictionary parameter                       
+1/-1     
ObjectExtensions.cs
Refactor DeepClone with modifier and centralized options 
+26/-40 
AgentService.GetAgents.cs
Add perInstanceCache parameter to SharpCache attribute     
+1/-1     
AgentService.LoadAgent.cs
Refactor agent loading with deep clone modifier                   
+40/-15 
AgentService.Rendering.cs
Use ConcurrentDictionary for thread-safe rendering data   
+45/-21 
AgentService.cs
Inject BotSharpOptions and remove local JSON options         
+9/-11   
TemplateRender.cs
Replace Dictionary with IDictionary parameter                       
+1/-1     
ComposeImageFn.cs
Refactor Init method and improve agent parameter handling
+17/-41 
EditImageFn.cs
Change Init method from async to synchronous                         
+2/-3     
ImageClientExtensions.cs
Refactor multipart request building with MultipartFormDataContent
+73/-81 
ImageCompletionProvider.Compose.cs
Pass model parameter to GenerateImageEdits method               
+1/-7     
ImageCompletionProvider.Edit.cs
Add quality parameter handling to image edit options         
+6/-0     
Bug fix
14 files
BotSharpOptions.cs
Add ReferenceHandler.IgnoreCycles to JSON options               
+3/-1     
AgentService.CreateAgent.cs
Use BotSharpOptions.JsonSerializerOptions for deserialization
+1/-1     
AgentService.RefreshAgents.cs
Use BotSharpOptions.JsonSerializerOptions for deserialization
+1/-1     
MCPToolAgentHook.cs
Replace .Result with ConfigureAwait async pattern               
+1/-1     
PluginLoader.cs
Replace .Result with ConfigureAwait async pattern               
+3/-3     
RoutingAgentHook.cs
Replace .Result with ConfigureAwait async pattern               
+1/-1     
ReasonerHelper.cs
Replace .Result with ConfigureAwait async pattern               
+1/-1     
RoutingContext.cs
Replace .Result with ConfigureAwait async pattern               
+4/-4     
ImageGenerationController.cs
Use IsNullOrEmpty check and fix error message                       
+2/-2     
Embeddings.cs
Replace .Result with ConfigureAwait async pattern               
+2/-2     
Images.cs
Replace .Result with ConfigureAwait async pattern               
+2/-2     
SqlPlannerAgentHook.cs
Replace .Result with ConfigureAwait async pattern               
+1/-1     
TwilioMessageQueueService.cs
Replace .Result with ConfigureAwait async pattern               
+1/-1     
PlaywrightWebDriver.cs
Replace .Result with ConfigureAwait async pattern               
+1/-1     
Formatting
2 files
HookEmitter.cs
Remove unused BotSharp.Abstraction.Hooks import                   
+0/-1     
InstructService.Execute.cs
Change Agent variable declaration from implicit to explicit
+1/-1     
Miscellaneous
2 files
ChatHubPlugin.cs
Remove unused imports and ChatHubCrontabHook registration
+0/-5     
ChatHubCrontabHook.cs
Delete unused ChatHubCrontabHook class                                     
+0/-62   
Tests
1 files
TestAgentService.cs
Update test service to use IDictionary and ConcurrentDictionary
+8/-7     

@iceljc iceljc requested a review from Oceania2018 October 20, 2025 20:02
@qodo-code-review
Copy link

PR Compliance Guide 🔍

Below is a summary of compliance checks for this PR:

Security Compliance
Possible auth/log exposure

Description: The code copies MultipartFormDataContent to a MemoryStream and sets the request content
without disposing the form until after, but builds headers from form.Headers; ensure
Authorization header is attached elsewhere (pipeline) or sensitive API keys are not
accidentally omitted or logged via exception messages that include response.Content.
ImageClientExtensions.cs [153-155]

Referred Code
    message.Request.Headers.Set("Content-Type", form.Headers.ContentType.ToString());
    message.Request.Content = BinaryContent.Create(BinaryData.FromStream(ms));
}
Template injection

Description: Rendering visibility expressions uses a copy of provided dictionary; if untrusted template
data can be injected into expressions, this may allow template-based injection; requires
confirmation of template engine sandboxing.
AgentService.Rendering.cs [158-166]

Referred Code
    var render = _services.GetRequiredService<ITemplateRender>();
    var copy = new Dictionary<string, object>(dict);
    var result = render.Render(visibilityExpression, new Dictionary<string, object>
    {
        { "states", copy }
    });

    return result.IsEqualTo("visible");
}
Ticket Compliance
🎫 No ticket provided
- [ ] Create ticket/issue <!-- /create_ticket --create_ticket=true -->

</details></td></tr>
Codebase Duplication Compliance
Codebase context is not defined

Follow the guide to enable codebase context checks.

Custom Compliance
No custom compliance provided

Follow the guide to enable custom compliance check.

Compliance status legend 🟢 - Fully Compliant
🟡 - Partial Compliant
🔴 - Not Compliant
⚪ - Requires Further Human Verification
🏷️ - Compliance label

@qodo-code-review
Copy link

PR Code Suggestions ✨

Explore these optional code suggestions:

CategorySuggestion                                                                                                                                    Impact
High-level
Replace .Result with async/await

Instead of replacing synchronous waits like .Result with
.ConfigureAwait(false).GetAwaiter().GetResult(), refactor the methods to be
fully asynchronous using async/await. This avoids thread blocking, improving
performance and code readability.

Examples:

src/Infrastructure/BotSharp.Core/MCP/Hooks/MCPToolAgentHook.cs [30]
        var functions = GetMcpContent(agent).ConfigureAwait(false).GetAwaiter().GetResult();
src/Infrastructure/BotSharp.Core/Plugins/PluginLoader.cs [162]
                var agent = agentService.LoadAgent(agentId).ConfigureAwait(false).GetAwaiter().GetResult();

Solution Walkthrough:

Before:

// In MCPToolAgentHook.cs
public override void OnAgentMcpToolLoaded(Agent agent)
{
    // ...
    var functions = GetMcpContent(agent).ConfigureAwait(false).GetAwaiter().GetResult();
    agent.SecondaryFunctions = agent.SecondaryFunctions.Concat(functions).DistinctBy(x => x.Name).ToList();
}

// In RoutingContext.cs
public string OriginAgentId
{
    get
    {
        // ...
        _routerAgentIds = agentService.GetAgents(...).ConfigureAwait(false).GetAwaiter().GetResult().Items.Select(x => x.Id).ToArray();
        // ...
    }
}

After:

// In MCPToolAgentHook.cs
public override async Task OnAgentMcpToolLoaded(Agent agent)
{
    // ...
    var functions = await GetMcpContent(agent);
    agent.SecondaryFunctions = agent.SecondaryFunctions.Concat(functions).DistinctBy(x => x.Name).ToList();
}

// In RoutingContext.cs
public async Task<string> GetOriginAgentId()
{
    if (_routerAgentIds == null)
    {
        var agentService = _services.GetRequiredService<IAgentService>();
        var agents = await agentService.GetAgents(...);
        _routerAgentIds = agents.Items.Select(x => x.Id).ToArray();
    }
    return _stack.Where(x => !_routerAgentIds.Contains(x)).LastOrDefault() ?? string.Empty;
}
Suggestion importance[1-10]: 8

__

Why: The suggestion correctly identifies that using .ConfigureAwait(false).GetAwaiter().GetResult() is a partial fix and that a full async/await refactoring is the ideal solution for performance and scalability, which is a significant architectural point.

Medium
Possible issue
Ensure correct LLM configuration is used

Update the agent's LlmConfig in GetImageEditResponse with the provider and model
from GetLlmProviderModel to ensure the correct fallback LLM configuration is
used.

src/Plugins/BotSharp.Plugin.ImageHandler/Functions/ComposeImageFn.cs [118-145]

 private async Task<string> GetImageEditResponse(Agent agent, string description)
 {
+    var (provider, model) = GetLlmProviderModel(agent);
+    agent.LlmConfig ??= new AgentLlmConfig();
+    agent.LlmConfig.ImageEdit ??= new ImageSetting();
+    agent.LlmConfig.ImageEdit.Provider = provider;
+    agent.LlmConfig.ImageEdit.Model = model;
+
     return await AiResponseHelper.GetImageGenerationResponse(_services, agent, description);
 }
 
 private (string, string) GetLlmProviderModel(Agent agent)
 {
     var provider = agent?.LlmConfig?.ImageEdit?.Provider;
     var model = agent?.LlmConfig?.ImageEdit?.Model;
 
     if (!string.IsNullOrEmpty(provider) && !string.IsNullOrEmpty(model))
     {
         return (provider, model);
     }
 
     provider = _settings?.Edit?.Provider;
     model = _settings?.Edit?.Model;
 
     if (!string.IsNullOrEmpty(provider) && !string.IsNullOrEmpty(model))
     {
         return (provider, model);
     }
 
     provider = "openai";
     model = "gpt-image-1-mini";
 
     return (provider, model);
 }
  • Apply / Chat
Suggestion importance[1-10]: 8

__

Why: The suggestion correctly identifies a potential bug where fallback LLM settings are not applied to the agent object, and provides a fix that ensures the correct configuration is used.

Medium
Learned
best practice
Replace blocking waits with awaits

Do not block on async calls; make the method async and await the operations to
prevent deadlocks and improve scalability.

src/Plugins/BotSharp.Plugin.MetaGLM/Modules/Embeddings.cs [38-39]

-var response = client.SendAsync(request, HttpCompletionOption.ResponseHeadersRead).ConfigureAwait(false).GetAwaiter().GetResult();
-var stream = response.Content.ReadAsStreamAsync().ConfigureAwait(false).GetAwaiter().GetResult();
+var response = await client.SendAsync(request, HttpCompletionOption.ResponseHeadersRead, CancellationToken.None);
+await using var stream = await response.Content.ReadAsStreamAsync(CancellationToken.None);
  • Apply / Chat
Suggestion importance[1-10]: 6

__

Why:
Relevant best practice - Avoid blocking on async calls; use non-blocking, deadlock-safe waits or async all the way.

Low
General
Simplify dictionary creation and remove redundancy

Refactor CollectRenderData to simplify dictionary initialization by removing a
redundant null check and directly constructing the dictionary from
agent.TemplateDict.

src/Infrastructure/BotSharp.Core/Agents/Services/AgentService.Rendering.cs [168-191]

 public ConcurrentDictionary<string, object> CollectRenderData(Agent agent)
 {
     var state = _services.GetRequiredService<IConversationStateService>();
 
-    var innerDict = new ConcurrentDictionary<string, object>();
-    if (agent?.TemplateDict != null)
-    {
-        var dict = new ConcurrentDictionary<string, object>(agent.TemplateDict);
-        if (dict != null)
-        {
-            foreach (var p in dict)
-            {
-                innerDict.AddOrUpdate(p.Key, p.Value, (key, curValue) => p.Value);
-            }
-        }
-    }
+    var innerDict = agent?.TemplateDict != null
+        ? new ConcurrentDictionary<string, object>(agent.TemplateDict)
+        : new ConcurrentDictionary<string, object>();
 
     foreach (var p in state.GetStates())
     {
         innerDict.AddOrUpdate(p.Key, p.Value, (key, curValue) => p.Value);
     }
 
     return innerDict;
 }
  • Apply / Chat
Suggestion importance[1-10]: 5

__

Why: The suggestion correctly identifies redundant and inefficient code in the CollectRenderData method and proposes a cleaner, more efficient implementation.

Low
  • More

@iceljc iceljc merged commit 34b2c91 into SciSharp:master Oct 20, 2025
3 of 4 checks passed
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