Change to concurrent dictionary

[iceljc]

PR Type

Enhancement

---

Description

• Replace Dictionary with ConcurrentDictionary for thread-safe template data handling

• Simplify agent loading by removing unnecessary deep cloning of agent properties

• Refactor render data collection to use standard dictionary operations instead of concurrent methods

• Update hook interfaces and implementations to use ConcurrentDictionary for instruction loading

• Remove redundant TemplateDict initialization in multiple agent creation scenarios

---

Diagram Walkthrough

flowchart LR
  A["Agent.TemplateDict"] -->|"Changed to"| B["ConcurrentDictionary"]
  C["IAgentHook.OnInstructionLoaded"] -->|"Parameter type"| B
  D["IAgentService.CollectRenderData"] -->|"Return type"| E["IDictionary"]
  F["LoadAgent"] -->|"Removed deep cloning"| G["Direct agent reference"]
  H["RenderInstruction/RenderTemplate"] -->|"Simplified operations"| E

Loading

File Walkthrough

	Relevant files
Enhancement	21 files AgentHookBase.cs Update hook base to use ConcurrentDictionary                          +2/-1      IAgentHook.cs Change OnInstructionLoaded parameter to ConcurrentDictionary +2/-1      IAgentService.cs Change CollectRenderData return type to IDictionary            +1/-2      Agent.cs Change TemplateDict to ConcurrentDictionary                            +2/-1      AgentService.LoadAgent.cs Remove deep cloning and simplify template dict handling    +3/-17    AgentService.Rendering.cs Refactor render data collection to use standard dictionaries +17/-20  RoutingAgentHook.cs Update hook implementation for ConcurrentDictionary parameter +2/-2      TranslationService.cs Update TemplateDict initialization to ConcurrentDictionary +5/-4      PlotChartFn.cs Simplify TemplateDict initialization with new syntax          +3/-3      SqlPrimaryStageFn.cs Remove unnecessary TemplateDict initialization                      +0/-1      SqlSecondaryStageFn.cs Remove unnecessary TemplateDict initialization                      +0/-1      SqlPlannerAgentHook.cs Update hook for ConcurrentDictionary and fix variable naming +6/-4      PrimaryStagePlanFn.cs Remove unnecessary TemplateDict initialization                      +0/-1      SecondaryStagePlanFn.cs Remove unnecessary TemplateDict initialization                      +0/-1      PyProgrammerFn.cs Simplify TemplateDict initialization with new syntax          +3/-3      ExecuteQueryFn.cs Remove unnecessary TemplateDict initialization                      +0/-1      VerifyDictionaryTerm.cs Remove unnecessary TemplateDict initialization                      +0/-1      SqlChartService.cs Update TemplateDict to use ConcurrentDictionary wrapper    +1/-1      VerifyDictionaryTerm.cs Remove unnecessary TemplateDict initialization                      +0/-1      CommonAgentHook.cs Update hook implementation for ConcurrentDictionary parameter +2/-1      PizzaBotAgentHook.cs Update hook implementation for ConcurrentDictionary parameter +2/-1
Formatting	1 files AgentService.GetAgents.cs Improve code formatting in agent loading methods                  +19/-4
Tests	1 files TestAgentService.cs Update test service to return IDictionary                                +2/-3

---

[qodo-code-review]

PR Compliance Guide 🔍

Below is a summary of compliance checks for this PR:

Security Compliance
🟢	No security concerns identified No security vulnerabilities detected by AI analysis. Human verification advised for critical code.
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.
Update

Compliance status legend

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

[qodo-code-review]

PR Code Suggestions ✨

Explore these optional code suggestions:

Category	Suggestion	Impact
Possible issue	Reintroduce deep cloning to prevent cache mutation Reintroduce the DeepClone call when loading an agent to prevent mutating the cached Agent instance and causing state leakage between requests. src/Infrastructure/BotSharp.Core/Agents/Services/AgentService.LoadAgent.cs [11-24] // [SharpCache(10, perInstanceCache: true)] public async Task<Agent> LoadAgent(string id, bool loadUtility = true) { if (string.IsNullOrEmpty(id) || id == Guid.Empty.ToString()) { return null; } HookEmitter.Emit<IAgentHook>(_services, hook => hook.OnAgentLoading(ref id), id); - var agent = await GetAgent(id); - if (agent == null) + var originalAgent = await GetAgent(id); + if (originalAgent == null) { return null; } + + var agent = originalAgent.DeepClone(); ... [To ensure code accuracy, apply this suggestion manually] Suggestion importance[1-10]: 10 __ Why: The suggestion correctly identifies a critical bug introduced by the PR where a cached object is mutated, which can lead to data corruption and unpredictable behavior across requests.	High
	Avoid sync-over-async call to prevent deadlocks Refactor the OnInstructionLoaded method to avoid a synchronous-over-asynchronous call, which can cause deadlocks. Use Task.WhenAll to run asynchronous operations in parallel and wait for them safely. src/Plugins/BotSharp.Plugin.Planner/SqlGeneration/Hooks/SqlPlannerAgentHook.cs [14-31] public override bool OnInstructionLoaded(string template, ConcurrentDictionary<string, object> dict) { var knowledgeHooks = _services.GetServices<IKnowledgeHook>(); + var dialog = new RoleDialogModel(AgentRole.User, template) + { + CurrentAgentId = PlannerAgentId.SqlPlanner + }; // Get global knowledges - var knowledges = new List<string>(); - foreach (var hook in knowledgeHooks) - { - var k = hook.GetGlobalKnowledges(new RoleDialogModel(AgentRole.User, template) - { - CurrentAgentId = PlannerAgentId.SqlPlanner - }).ConfigureAwait(false).GetAwaiter().GetResult(); - knowledges.AddRange(k); - } + var tasks = knowledgeHooks.Select(hook => hook.GetGlobalKnowledges(dialog)).ToList(); + Task.WhenAll(tasks).ConfigureAwait(false).GetAwaiter().GetResult(); + + var knowledges = tasks.SelectMany(task => task.Result).ToList(); dict["global_knowledges"] = knowledges; return true; } Apply / Chat Suggestion importance[1-10]: 7 __ Why: The suggestion correctly identifies a risky sync-over-async call and proposes a safer, more efficient alternative using Task.WhenAll that avoids potential deadlocks.	Medium
High-level	Re-evaluate removal of agent deep cloning The removal of deep cloning in LoadAgent is risky. If GetAgent returns a cached object, modifying it directly will cause race conditions across concurrent requests. It is recommended to verify state isolation or reintroduce cloning. Examples: src/Infrastructure/BotSharp.Core/Agents/Services/AgentService.LoadAgent.cs [20] var agent = await GetAgent(id); Solution Walkthrough: Before: public async Task<Agent> LoadAgent(string id, bool loadUtility = true) { // ... var agent = await GetAgent(id); // GetAgent is cached, so 'agent' might be a shared instance. if (agent == null) { return null; } // These operations mutate the potentially shared 'agent' object directly. agent.TemplateDict = []; agent.SecondaryInstructions = []; agent.SecondaryFunctions = []; await InheritAgent(agent); // ... more mutations return agent; } After: public async Task<Agent> LoadAgent(string id, bool loadUtility = true) { // ... var originalAgent = await GetAgent(id); // Get the potentially shared, cached agent. if (originalAgent == null) { return null; } // Create a deep clone to work with, isolating this request's state. var agent = originalAgent.DeepClone(/* ... */); // Mutations are now safe on the cloned instance. agent.TemplateDict = []; agent.SecondaryInstructions = []; agent.SecondaryFunctions = []; await InheritAgent(agent); // ... more mutations return agent; } Suggestion importance[1-10]: 9 __ Why: This suggestion correctly identifies a critical potential for race conditions by removing DeepClone on a cached object, which could severely impact application stability and correctness in a concurrent environment.	High
General	Simplify and improve dictionary merging logic Refactor the CollectRenderData method to simplify its dictionary merging logic for better readability and conciseness. src/Infrastructure/BotSharp.Core/Agents/Services/AgentService.Rendering.cs [167-188] public IDictionary<string, object> CollectRenderData(Agent agent) { var state = _services.GetRequiredService<IConversationStateService>(); - var innerDict = new Dictionary<string, object>(); - var dict = new Dictionary<string, object>(agent.TemplateDict ?? []); - if (dict != null) - { - foreach (var p in dict) - { - innerDict[p.Key] = p.Value; - } - } + // Start with the agent's template dictionary. + var innerDict = new Dictionary<string, object>(agent.TemplateDict ?? new ConcurrentDictionary<string, object>()); - var states = new Dictionary<string, string>(state.GetStates()); + // Overwrite with current conversation states, which are more specific. + var states = state.GetStates(); foreach (var p in states) { innerDict[p.Key] = p.Value; } return innerDict; } Apply / Chat Suggestion importance[1-10]: 5 __ Why: The suggestion offers a valid refactoring that simplifies the dictionary merging logic, making the code more concise and readable without changing its behavior.	Low
Update

[hchen2020]

Should use IDictionary ?