Skip to content

.pr_agent_auto_best_practices

Jump to bottom
qodo-merge-bot edited this page Jul 3, 2026 · 11 revisions

Pattern 1: Defensively copy mutable collections/dictionaries and cached objects before storing, returning, or mutating them to prevent shared-state leaks across layers/requests.

Example code before:

// stores caller-owned list by reference
message.Parts = parts;

// returns cached mutable object directly
return _cache.GetOrCreate(key, () => template);

// mutates caller-owned dictionary
options.Data["input"] = malformedJson;

Example code after:

message.Parts = parts?.ToList() ?? new List<Part>();

var cached = _cache.GetOrCreate(key, () => LoadTemplate());
return cached is null ? null : cached.DeepClone(); // or new AgentTemplate(cached)

var data = options?.Data is null ? new Dictionary<string, object>()
                                 : new Dictionary<string, object>(options.Data);
data["input"] = malformedJson;
Relevant past accepted suggestions:
Suggestion 1: [correctness] `parts` not defensively copied
`parts` not defensively copied `SendMessageStreamingAsync` assigns the caller-provided `List` directly to `Message.Parts`, allowing shared mutable state across layers and potential unexpected mutation. This violates the defensive-copy requirement for caller-provided collections.

Issue description

SendMessageStreamingAsync stores the caller-provided List<Part> by reference (Parts = parts), which can leak mutable state across components.

Issue Context

Even if parts is not mutated locally, downstream code/libraries may mutate it; copying prevents cross-request/shared-state bugs.

Fix Focus Areas

  • src/Infrastructure/BotSharp.Core.A2A/Services/A2AService.cs[124-131]
Suggestion 2: [reliability] Cached `AgentTemplate` returned mutable
Cached `AgentTemplate` returned mutable `GetAgentTemplateDetail` is cached and returns a mutable `AgentTemplate` instance directly, so downstream callers can mutate the cached object and leak state across requests/components. This violates the requirement to defensively copy cached/caller-owned mutable objects before exposing them to downstream code.

Issue description

GetAgentTemplateDetail is cached and returns a mutable AgentTemplate instance directly; callers can mutate it and affect subsequent cache hits.

Issue Context

The compliance requirement calls out cached mutable objects as a key source of cross-request/component state leakage.

Fix Focus Areas

  • src/Infrastructure/BotSharp.Core/Agents/Services/AgentService.GetAgents.cs[108-136]
Suggestion 3: [reliability] `GetGraphInfo()` leaks internal lists
`GetGraphInfo()` leaks internal lists `RuleGraph.GetGraphInfo()` returns internal `_nodes`/`_edges` list references, allowing external mutation and cross-call side effects. This violates the defensive-copy requirement for caller/response collections before exposure or mutation.

Issue description

RuleGraph.GetGraphInfo() exposes internal mutable _nodes and _edges lists via RuleGraphInfo, enabling external callers to mutate internal state.

Issue Context

Compliance requires defensive copies of collections before exposing/assigning them to prevent shared mutable references.

Fix Focus Areas

  • src/Infrastructure/BotSharp.Abstraction/Agents/Models/RuleGraph.cs[133-140]
Suggestion 4: [reliability] `AddEdge()` stores shared collections
`AddEdge()` stores shared collections `RuleGraph.AddEdge()` assigns `payload.Labels` and `payload.Config` directly into the new `RuleEdge`, preserving shared mutable references. Mutations to the payload collections after the call can unintentionally mutate graph edges.

Issue description

RuleGraph.AddEdge() assigns collection properties from payload directly onto the edge, creating shared mutable references.

Issue Context

Compliance requires defensive copies of caller/response collections before mutation or assignment to prevent cross-request side effects.

Fix Focus Areas

  • src/Infrastructure/BotSharp.Abstraction/Agents/Models/RuleGraph.cs[106-115]
Suggestion 5: [reliability] `OnRoutingRulesLoaded` mutates cached rules
`OnRoutingRulesLoaded` mutates cached rules `OnRoutingRulesLoaded` is invoked with a mutable list of `RoutingRule` objects that originate from cached/shared agent routing rule instances, so hook mutations can leak across requests. This violates the defensive-copy requirement and can cause hard-to-debug cross-request side effects.

Issue description

OnRoutingRulesLoaded receives a mutable list of RoutingRule instances that are sourced from GetRoutingRecords(). Because GetRoutingRecords() is cached and returns agent-owned RoutingRule objects, in-place mutations by hooks can leak across requests.

Issue Context

The current code uses ToList() which only copies the list container, not the contained RoutingRule objects. The hook contract explicitly encourages in-place mutation.

Fix Focus Areas

  • src/Infrastructure/BotSharp.Core/Routing/RoutingService.cs[150-170]
Suggestion 6:

Copy mutable dictionaries before mutation

Avoid mutating options.Data directly (it may be reused across requests); create a new dictionary copy before adding/updating keys.

src/Infrastructure/BotSharp.Core/Shared/JsonRepairService.cs [92-95]

 var render = _services.GetRequiredService<ITemplateRender>();
-var data = options?.Data ?? [];
+var data = options?.Data is null
+    ? new Dictionary<string, object>()
+    : new Dictionary<string, object>(options.Data);
 data["input"] = malformedJson;
 var prompt = render.Render(template, data);
Suggestion 7:

Prevent shared mutable metadata

To prevent shared state bugs from mutable dictionaries, create a new MetaData dictionary instance for the message instead of assigning it by reference from response.MetaData.

src/Infrastructure/BotSharp.Core/Routing/RoutingService.InvokeAgent.cs [76-79]

 message = RoleDialogModel.From(message, role: AgentRole.Assistant, content: response.Content);
 message.CurrentAgentId = agent.Id;
-message.MetaData = response.MetaData;
+message.MetaData = response.MetaData != null ? new(response.MetaData) : null;
 message.IsStreaming = response.IsStreaming;
Suggestion 8:

Avoid shared metadata reference

Create a new dictionary instance for message.FunctionMetaData instead of assigning by reference to prevent potential shared state issues.

src/Infrastructure/BotSharp.Core/Routing/RoutingService.InvokeAgent.cs [59]

-message.FunctionMetaData = response.FunctionMetaData;
+message.FunctionMetaData = response.FunctionMetaData != null ? new(response.FunctionMetaData) : null;

Pattern 2: Add explicit null/empty validation and safe failure behavior at API/integration boundaries (request bodies, endpoints/URLs, WebSocket state, config values) instead of relying on exceptions.

Example code before:

var uri = new Uri(agentEndpoint); // may throw if null/empty

public IActionResult Save([FromBody] SaveRequest request)
    => Ok(_svc.Save(request.Thumbnail)); // request may be null

await webSocket.SendAsync(bytes, ...); // may throw if not open

Example code after:

if (string.IsNullOrWhiteSpace(agentEndpoint))
    return Result.Fail("agentEndpoint is required");
var uri = new Uri(agentEndpoint);

public IActionResult Save([FromBody] SaveRequest? request)
{
    if (request?.Thumbnail is null) return BadRequest("thumbnail is required");
    return Ok(_svc.Save(request.Thumbnail));
}

if (webSocket.State == WebSocketState.Open)
    await webSocket.SendAsync(bytes, ...);
Relevant past accepted suggestions:
Suggestion 1: [reliability] `agentEndpoint` missing null guard
`agentEndpoint` missing null guard Integration-boundary methods build `new Uri(agentEndpoint)` without validating for null/empty, which can throw and fail requests unexpectedly. This violates the null/empty guard requirement for boundary inputs.

Issue description

agentEndpoint/endPoint are used to create new Uri(...) without null/empty validation, which can throw at runtime.

Issue Context

These methods are integration boundaries (remote A2A endpoints). Per compliance, they should guard inputs and return a safe fallback (or explicitly fail fast with a clear error) rather than crashing via unhandled Uri construction errors.

Fix Focus Areas

  • src/Infrastructure/BotSharp.Core.A2A/Services/A2AService.cs[39-56]
Suggestion 2: [correctness] Filtered route returns unfiltered
Filtered route returns unfiltered GetRuleTriggers(string agentId) skips filtering when agentId is whitespace, so calls to the filtered route /rule/triggers/{agentId} can return the full unfiltered trigger list. This makes invalid route values indistinguishable from the explicit /rule/triggers endpoint and can hide client/request bugs.

Issue description

GET /rule/triggers/{agentId} currently returns all triggers when agentId is whitespace, making the filtered route behave like the unfiltered GET /rule/triggers endpoint for invalid inputs.

Issue Context

This endpoint already has an explicit unfiltered overload (GET /rule/triggers). A whitespace/invalid {agentId} should be treated as a client error (e.g., 400) or the endpoint should always apply filtering.

Fix Focus Areas

  • src/Infrastructure/BotSharp.OpenAPI/Controllers/Agent/AgentController.Rule.cs[8-15]

Suggested change

  • Replace the if (!string.IsNullOrWhiteSpace(agentId)) { ... } guard with:
  • if (string.IsNullOrWhiteSpace(agentId)) return BadRequest("agentId is required"); (change return type to ActionResult<IEnumerable<AgentRuleViewModel>>), or
  • remove the guard and always apply the filter for this route.
Suggestion 3: [reliability] `request` not null-checked
`request` not null-checked `SaveConversationThumbnail` dereferences `request.Thumbnail` without guarding against a null body, which can throw at this API boundary. This violates the requirement to add null/empty guards and safe fallbacks for boundary inputs.

Issue description

SaveConversationThumbnail dereferences request.Thumbnail without guarding request against null, which can throw when the request body is missing/invalid.

Issue Context

This is an API boundary ([FromBody] binding). The compliance rules require explicit null/empty guards and safe fallbacks at system boundaries.

Fix Focus Areas

  • src/Infrastructure/BotSharp.OpenAPI/Controllers/Conversation/ConversationController.File.cs[121-133]
Suggestion 4: [correctness] Silent default DB fallback
Silent default DB fallback `GetDatabaseName` now falls back to the hard-coded database name `BotSharp` when both `MongoUrl.DatabaseName` and `MongoUrl.AuthenticationSource` are empty. This can silently send all Mongo storage reads/writes and collection/index creation to an unintended database, masking misconfiguration and potentially mixing data.

Issue description

GetDatabaseName silently falls back to the hard-coded database name BotSharp when the MongoDB URL does not contain a database (and no auth source is provided). This can route all MongoRepository reads/writes and collection/index creation into an unintended database.

Issue Context

MongoDbContext uses the derived DB name to build Database, and GetCollectionOrCreate will create collections in that database. If the DB name fallback is hit, the application can succeed while persisting data to the wrong place.

Fix Focus Areas

  • src/Plugins/BotSharp.Plugin.MongoStorage/MongoDbContext.cs[24-30]
  • src/Plugins/BotSharp.Plugin.MongoStorage/MongoDbContext.cs[30-67]
Suggestion 5: [reliability] Missing config guardrails
Missing config guardrails `MongoDbContext` constructs `MongoClient` and parses the connection string without any local validation, even though `BotSharpMongoDb` can be empty by default. Add explicit validation (and a clear error message) so misconfiguration fails fast and predictably when MongoRepository is enabled.

Issue description

When Database:Default is set to MongoRepository, the system constructs MongoDbContext even if BotSharpMongoDb is empty. There is no local validation or clear configuration error.

Issue Context

BotSharpMongoDb defaults to empty string in settings, and appsettings includes an empty default value. The plugin does not validate before instantiating MongoDbContext, and MongoDbContext immediately uses the value.

Fix Focus Areas

  • src/Plugins/BotSharp.Plugin.MongoStorage/MongoStoragePlugin.cs[19-57]
  • src/Plugins/BotSharp.Plugin.MongoStorage/MongoDbContext.cs[15-28]
  • src/Infrastructure/BotSharp.Abstraction/Repositories/Settings/BotSharpDatabaseSettings.cs[3-12]
Suggestion 6:

Add null checks and fallback

Guard against router or response.Content being null (e.g., agent not found, provider errors) and fall back safely to the original JSON while logging the reason.

src/Infrastructure/BotSharp.Core/JsonRepair/JsonRepairService.cs [58-96]

 var agentService = _services.GetRequiredService<IAgentService>();
 var router = await agentService.GetAgent(ROUTER_AGENT_ID);
+if (router == null)
+{
+    _logger.LogWarning("Agent '{AgentId}' not found; returning original JSON.", ROUTER_AGENT_ID);
+    return malformedJson;
+}
 
 var template = router.Templates?.FirstOrDefault(x => x.Name == TEMPLATE_NAME)?.Content;
 if (string.IsNullOrEmpty(template))
 {
-    _logger.LogWarning($"Template '{TEMPLATE_NAME}' not found in agent '{ROUTER_AGENT_ID}'");
+    _logger.LogWarning("Template '{TemplateName}' not found in agent '{AgentId}'; returning original JSON.", TEMPLATE_NAME, ROUTER_AGENT_ID);
     return malformedJson;
 }
 ...
 var response = await completion.GetChatCompletions(agent, dialogs);
 
-_logger.LogInformation($"JSON repair result: {response.Content}");
-return response.Content;
+var content = response?.Content;
+if (string.IsNullOrWhiteSpace(content))
+{
+    _logger.LogWarning("JSON repair returned empty content; returning original JSON.");
+    return malformedJson;
+}
 
+_logger.LogInformation("JSON repair result: {Json}", content);
+return content;
+
Suggestion 7:

Prevent crash when no models match

Prevent a potential ArgumentOutOfRangeException in GetProviderModel by checking if the filtered models collection is empty. If it is, return null instead of attempting to select an element, which would cause a crash.

src/Infrastructure/BotSharp.Core/Infrastructures/LlmProviderService.cs [61-69]

 if (capabilities != null)
 {
     models = models.Where(x => x.Capabilities != null && capabilities.Any(y => x.Capabilities.Contains(y)));
 }
 
+var availableModels = models.ToList();
+if (!availableModels.Any())
+{
+    return null;
+}
+
 var random = new Random();
-var index = random.Next(0, models.Count());
-var modelSetting = models.ElementAt(index);
+var index = random.Next(0, availableModels.Count);
+var modelSetting = availableModels.ElementAt(index);
 return modelSetting;
Suggestion 8:

Check WebSocket state

The SendEventToUser method doesn't check if the WebSocket is in a valid state before sending data. If the client has disconnected or the connection is closing, this could throw an exception and crash the middleware.

src/Plugins/BotSharp.Plugin.ChatHub/ChatStreamMiddleware.cs [106]

-await SendEventToUser(webSocket, data);
+if (webSocket.State == WebSocketState.Open)
+{
+    await SendEventToUser(webSocket, data);
+}

[Suggestion has been applied]

Suggestion 9:

Prevent null reference exception

The code uses a null conditional operator on agent but doesn't check if agent is null before accessing its Description property. If agent is null, this could lead to a NullReferenceException when trying to access agent.Description.

src/Plugins/BotSharp.Plugin.OpenAI/Providers/Realtime/RealTimeCompletionProvider.cs [283]

-var instruction = messages.FirstOrDefault()?.Content.FirstOrDefault()?.Text ?? agent?.Description;
+var instruction = messages.FirstOrDefault()?.Content.FirstOrDefault()?.Text ?? agent?.Description ?? string.Empty;
Suggestion 10:

Remove unnecessary async and add guard

The method is declared async but contains no await, which can introduce unnecessary state machine overhead and warnings. Also, accessing page.Url may throw if the page is disposed; wrap in a try-catch and return an empty string on failure to align with the null-path behavior.

src/Plugins/BotSharp.Plugin.WebDriver/Drivers/PlaywrightDriver/PlaywrightWebDriver.ExtractData.cs [20-26]

-public async Task<string> GetCurrentUrl(MessageInfo message)
+public Task<string> GetCurrentUrl(MessageInfo message)
 {
-    var page = _instance.GetPage(message.ContextId);
-    if (page == null)
-        return string.Empty;
-    return page.Url;
+    try
+    {
+        var page = _instance.GetPage(message.ContextId);
+        if (page == null) return Task.FromResult(string.Empty);
+        return Task.FromResult(page.Url ?? string.Empty);
+    }
+    catch
+    {
+        return Task.FromResult(string.Empty);
+    }
 }

Pattern 3: Use ordinal, case-insensitive comparison semantics for identifier-like strings (template names, model IDs, function names) and build collections with the comparer rather than passing comparers to APIs incorrectly.

Example code before:

if (x.TemplateName == name) { ... }

models = models.Where(m => filter.ModelIds.Contains(m.Id)); // case-sensitive

var words = new HashSet<string>(tokens);
functions = functions.Where(f => words.Contains(f.Name, StringComparer.OrdinalIgnoreCase)); // invalid overload

Example code after:

if (string.Equals(x.TemplateName, name, StringComparison.OrdinalIgnoreCase)) { ... }

models = models.Where(m => filter.ModelIds.Contains(m.Id, StringComparer.OrdinalIgnoreCase));

var words = new HashSet<string>(tokens, StringComparer.OrdinalIgnoreCase);
functions = functions.Where(f => !string.IsNullOrEmpty(f.Name) && words.Contains(f.Name));
Relevant past accepted suggestions:
Suggestion 1: [correctness] Case-sensitive `TemplateName` match
Case-sensitive `TemplateName` match Template selection compares identifier-like names with `==`, which is case-sensitive and can behave inconsistently for identifier matching. This violates the requirement to use ordinal, case-insensitive comparisons for identifiers.

Issue description

Template lookup uses a case-sensitive equality operator for an identifier-like string (TemplateName), risking mismatches.

Issue Context

Identifier comparisons must be culture-invariant and case-insensitive.

Fix Focus Areas

  • src/Infrastructure/BotSharp.Core/Instructs/Services/InstructService.Instruct.cs[69-69]
Suggestion 2: [correctness] Culture-based model name compare
Culture-based model name compare Model identifier matching uses `StringComparison.InvariantCultureIgnoreCase`, which is culture-sensitive and can cause inconsistent matches for identifier-like strings. This violates the requirement to use robust comparers for identifier matching.

Issue description

Model-name matching uses a culture-based comparison (InvariantCultureIgnoreCase) which is not robust for identifier matching and may yield inconsistent results.

Issue Context

Model names (like gpt-4o) are identifiers and should typically be compared using ordinal semantics to avoid culture-specific casing behavior.

Fix Focus Areas

  • src/Infrastructure/BotSharp.Core/Infrastructures/SettingService.cs[57-57]
Suggestion 3:

Use case-insensitive model ID filtering

Update the model ID filtering logic in GetLlmConfigs to be case-insensitive. Use StringComparer.OrdinalIgnoreCase when checking if filter.ModelIds contains a model's ID to ensure consistent and robust filtering.

src/Infrastructure/BotSharp.Core/Infrastructures/LlmProviderService.cs [139-142]

 if (filter.ModelIds != null)
 {
-    models = models.Where(x => filter.ModelIds.Contains(x.Id));
+    models = models.Where(x => filter.ModelIds.Contains(x.Id, StringComparer.OrdinalIgnoreCase));
 }
Suggestion 4:

Fix comparer misuse and null checks

Avoid passing a comparer to HashSet.Contains — it only accepts the element. Also, null-check function names to prevent NREs. Build the set with the comparer and call Contains with a single argument, and ensure x.Name is not null before accessing.

src/Infrastructure/BotSharp.Core/Agents/Services/AgentService.Rendering.cs [110-124]

 public IEnumerable<FunctionDef> FilterFunctions(string instruction, Agent agent, StringComparer? comparer = null)
 {
     var functions = agent.Functions.AsEnumerable();
 
     if (agent.FuncVisMode.IsEqualTo(AgentFuncVisMode.Auto) && !string.IsNullOrWhiteSpace(instruction))
     {
-        comparer = comparer ?? StringComparer.OrdinalIgnoreCase;
+        comparer ??= StringComparer.OrdinalIgnoreCase;
         var matches = Regex.Matches(instruction, @"\b[A-Za-z0-9_]+\b");
         var words = new HashSet<string>(matches.Select(m => m.Value), comparer);
-        functions = functions.Where(x => words.Contains(x.Name, comparer));
+        functions = functions.Where(x => !string.IsNullOrEmpty(x.Name) && words.Contains(x.Name));
     }
 
     functions = functions.Concat(agent.SecondaryFunctions ?? []);
     return functions;
 }

Pattern 4: When introducing new domain fields or request payload fields, ensure persistence/storage mappings and conversation-state propagation are updated so values round-trip correctly.

Example code before:

// patch persists only part of the model
foundTemplate.Content = template.Content;
// ResponseFormat/LlmConfig dropped

// new domain property not mapped
return new Doc { Title = item.Title }; // missing ReentryProtection

// function args not persisted
element.FunctionName = dialog.FunctionName;
// element.FunctionArgs missing

Example code after:

foundTemplate.Content = template.Content;
foundTemplate.ResponseFormat = template.ResponseFormat;
foundTemplate.LlmConfig = ToMongo(template.LlmConfig);

return new Doc { Title = item.Title, ReentryProtection = item.ReentryProtection };

element.FunctionName = dialog.FunctionName;
element.FunctionArgs = dialog.FunctionArgs;
Relevant past accepted suggestions:
Suggestion 1: [correctness] Routing args dropped
Routing args dropped RoutingService.InstructLoop no longer persists inst.Arguments into IConversationStateService, so router-extracted required parameters are not available to later routing/agent execution. This can cause HasMissingRequiredField to falsely report missing required fields and prevents agents/templates from seeing values that are normally sourced from conversation state.

Issue description

RoutingService.InstructLoop no longer calls states.SaveStateByArgs(inst.Arguments), which was the only mechanism that propagated the router/planner-produced JSON args into conversation state. Router instructions explicitly require populating required agent args under args, and downstream logic relies on those values being in state (for prompt rendering and required-field auto-fill).

Issue Context

  • Router instruction requires: “Extract and populate agent required arguments” (these appear under the JSON args object).
  • HasMissingRequiredField uses conversation state to fill missing required fields.
  • Agent prompt/template render data is populated from conversation state.

Fix Focus Areas

Choose one approach, but ensure routing-required parameters from inst.Arguments still reach:

  1. required-field evaluation (HasMissingRequiredField) and
  2. agent/template rendering (state-driven). Recommended options:
  • Option A (preferred with PR intent): Stop persisting routing args as long-lived state, but update HasMissingRequiredField to also read required-field values from nested args in the routing instruction payload (e.g., when message.FunctionArgs is a serialized FunctionCallFromLlm), and merge those values into the working args object for the missing-field check.
  • Option B: Persist only the agent routing-rule fields from inst.Arguments with a limited lifetime (e.g., activeRounds=1..2) instead of saving every arg indefinitely; add a new helper API rather than reusing SaveStateByArgs if you need TTL. References:
  • src/Infrastructure/BotSharp.Core/Routing/RoutingService.InstructLoop.cs[46-62]
  • src/Infrastructure/BotSharp.Core/Routing/RoutingService.HasMissingRequiredField.cs[12-68]
  • src/Infrastructure/BotSharp.Core/Agents/Services/AgentService.Rendering.cs[181-197]
  • src/Infrastructure/BotSharp.Core/Conversations/Services/ConversationStateService.cs[382-410]
  • src/Infrastructure/BotSharp.Core/data/agents/01fcc3e5-9af7-49e6-ad7a-a760bd12dc4a/instructions/instruction.liquid[3-9]
Suggestion 2: [correctness] Token cache not persisted
Token cache not persisted A2AService stores ResponseContinuationToken only in a private in-memory dictionary, but messages are processed through per-request HTTP endpoints and IA2AService is registered as scoped, so the continuation token will be lost between user messages and subsequent calls won’t include session continuation.

Issue description

A2AService stores ResponseContinuationToken in an in-memory dictionary. Since IA2AService is registered as scoped and chat messages are processed by per-request HTTP endpoints, a new A2AService instance is created for each user message and the token cache is lost. This prevents A2A multi-turn session continuation.

Issue Context

  • A2ADelegationFn passes conversationId as contextId, implying multi-turn continuity is expected.
  • A2AService only continues a session when it can retrieve a previously cached continuation token.

Fix Focus Areas

  • src/Infrastructure/BotSharp.Core.A2A/Services/A2AService.cs[23-110]
  • src/Infrastructure/BotSharp.Core.A2A/A2APlugin.cs[24-35]
  • src/Infrastructure/BotSharp.Core.A2A/Functions/A2ADelegationFn.cs[45-58]

Implementation direction (choose one)

  1. Persist tokens in conversation state (recommended): serialize/store the continuation token in IConversationStateService (or another durable per-conversation store) keyed by (agentEndpoint, conversationId) and restore it when building AgentRunOptions.
  2. Make session cache truly long-lived: change IA2AService lifetime to singleton and make the caches thread-safe + bounded (e.g., ConcurrentDictionary + eviction/TTL) to avoid unbounded growth.
Suggestion 3: [correctness] Mongo patch drops config
Mongo patch drops config MongoRepository.PatchAgentTemplate updates only Content, so template ResponseFormat/LlmConfig changes sent via /agent/{agentId}/templates are silently lost when Mongo storage is used.

Issue description

MongoRepository.PatchAgentTemplate persists only Content, dropping new per-template fields (ResponseFormat, LlmConfig) when templates are patched.

Issue Context

/agent/{agentId}/templates uses AgentTemplate as request payload, so clients can submit responseFormat/llmConfig but Mongo storage will ignore them.

Fix Focus Areas

  • src/Plugins/BotSharp.Plugin.MongoStorage/Repository/MongoRepository.Agent.cs[574-597]

Implementation notes

Update the found template element to include:

  • foundTemplate.ResponseFormat = template.ResponseFormat;
  • foundTemplate.LlmConfig = AgentTemplateLlmConfigMongoModel.ToMongoModel(template.LlmConfig); (or replace the element in the list with AgentTemplateMongoElement.ToMongoElement(template)), then persist the updated templates array.
Suggestion 4: [correctness] Reentry flag not persisted
Reentry flag not persisted CrontabItem.ReentryProtection is added to the domain model, but Mongo persistence mapping omits it, so the value cannot round-trip and will silently revert to the default on load.

Issue description

CrontabItem.ReentryProtection was added to the domain model, but Mongo persistence (CrontabItemDocument + mapping methods) does not include this field, so disabling protection (or setting it explicitly) will not persist.

Issue Context

CrontabItemDocument.ToDomainModel / ToMongoModel are manually mapping fields; any new domain property must be mapped explicitly.

Fix Focus Areas

  • src/Infrastructure/BotSharp.Abstraction/Crontab/Models/CrontabItem.cs[32-42]
  • src/Plugins/BotSharp.Plugin.MongoStorage/Collections/CrontabItemDocument.cs[5-62]

Implementation notes

  • Add public bool ReentryProtection { get; set; } to CrontabItemDocument.
  • Map it in both directions:
  • ToDomainModel: ReentryProtection = item.ReentryProtection
  • ToMongoModel: ReentryProtection = item.ReentryProtection
  • If there are existing stored docs, ensure the default behavior is acceptable when the field is missing (Mongo default false unless handled); consider defaulting to true when absent if that matches intended behavior.
Suggestion 5:

Persist function arguments in storage

Restore the assignment of FunctionArgs in BuildDialogElement to prevent losing function call arguments when persisting conversation history.

src/Infrastructure/BotSharp.Core/Conversations/Services/ConversationStorage.cs [114-117]

 ToolCallId = dialog.ToolCallId,
 FunctionName = dialog.FunctionName,
+FunctionArgs = dialog.FunctionArgs,
 FunctionMetaData = dialog.FunctionMetaData,
 CreatedTime = dialog.CreatedAt

Pattern 5: Make background/retry flows observable and safe: log exceptions from fire-and-forget work, avoid duplicating side effects (duplicate dialogs/hooks), and on LLM retries adjust inputs to guide correction rather than resubmitting unchanged prompts.

Example code before:

Task.Run(() => DoWork()); // exceptions unobserved

_logger.LogWarning("LLM output: {Text}", text); // logs sensitive content

// retry with identical prompt
options.IsRetry = true;
return await InvokeAgent(agentId, dialogs, options);

dialogs.Add(message);
Context.AddDialogs([message]); // duplicate history entry

Example code after:

_ = Task.Run(async () =>
{
    try { await DoWorkAsync(); }
    catch (Exception ex) { _logger.LogError(ex, "Background work failed"); }
});

_logger.LogWarning("LLM response invalid (finish_reason={Reason})", reason); // no raw text

var retryDialogs = dialogs.ToList();
retryDialogs.Add(new RoleDialogModel(AgentRole.System,
    "Your previous response was invalid. Return a valid function call with a function name."));
return await InvokeAgent(agentId, retryDialogs, new InvokeAgentOptions { IsRetry = true });

Context.AddDialogs([message]); // only one append path
Relevant past accepted suggestions:
Suggestion 1: [reliability] Fire-and-forget exceptions unlogged
Fire-and-forget exceptions unlogged SchedulingCrontab uses Task.Run without awaiting and the new ExecuteTimeArrivedItem wrapper no longer catches/logs exceptions, so failures (e.g., DI resolution) can become unobserved task exceptions with no application logging.

Issue description

SchedulingCrontab fires Task.Run(...) and does not observe the returned Task. The refactored ExecuteTimeArrivedItem no longer wraps execution in try/catch, so exceptions can be lost (or only surface as unobserved task exceptions).

Issue Context

Even if ExecuteTimeArrivedItemWithReentryProtection catches Redis/lock errors, exceptions can still occur before entering it (scope creation / DI resolution).

Fix Focus Areas

  • src/Infrastructure/BotSharp.OpenAPI/Controllers/Crontab/CrontabController.cs[59-66]
  • src/Infrastructure/BotSharp.OpenAPI/Controllers/Crontab/CrontabController.cs[87-92]

Implementation notes

  • Prefer _ = Task.Run(async () => { try { await ExecuteTimeArrivedItem(item, _services); } catch (Exception ex) { _logger.LogError(ex, "Crontab: {Title} background execution failed", item.Title); } });
  • Alternatively, avoid Task.Run and use a background queue/hosted service (if available in this codebase) so execution is tracked and failures are logged.
Suggestion 2:

Retry logic should guide the LLM

The current retry logic resubmits the same dialog history, which is unreliable. It is suggested to modify the prompt on retry by adding a system message that instructs the LLM to correct its previous invalid output.

Examples:

src/Infrastructure/BotSharp.Core/Routing/RoutingService.InvokeAgent.cs [162-172]

        if (string.IsNullOrWhiteSpace(message.FunctionName))
        {
            if (!(options?.IsRetry ?? false))
            {
                // Retry once by recursively calling InvokeAgent
                _logger.LogWarning($"Function name is empty, retrying InvokeAgent for agent {agentId}");
                options ??= InvokeAgentOptions.Default();
                options.IsRetry = true;
                var retryResult = await InvokeAgent(agentId, dialogs, options);
                return retryResult;

 ... (clipped 1 lines)

Solution Walkthrough:

Before:

private async Task HandleEmptyFunctionNameRetry(...)
{
    if (string.IsNullOrWhiteSpace(message.FunctionName))
    {
        if (!options.IsRetry)
        {
            // Retry once by recursively calling InvokeAgent
            _logger.LogWarning("Function name is empty, retrying...");
            options.IsRetry = true;
            // The exact same 'dialogs' are passed again.
            var retryResult = await InvokeAgent(agentId, dialogs, options);
            return retryResult;
        }
        else
        {
            // ... handle failure after retry
        }
    }
    return null;
}

After:

private async Task HandleEmptyFunctionNameRetry(...)
{
    if (string.IsNullOrWhiteSpace(message.FunctionName))
    {
        if (!options.IsRetry)
        {
            _logger.LogWarning("Function name is empty, retrying with guidance...");
            options.IsRetry = true;
            
            // Add a corrective message to guide the LLM on retry.
            var retryDialogs = new List(dialogs);
            retryDialogs.Add(new RoleDialogModel(AgentRole.System, 
                "Your previous response was invalid because the function name was missing. Please provide a valid function call."));

            var retryResult = await InvokeAgent(agentId, retryDialogs, options);
            return retryResult;
        }
        else
        {
            // ... handle failure after retry
        }
    }
    return null;
}
Suggestion 3:

Prevent duplicate dialog message entry

Remove the redundant dialogs.Add(message) and Context.AddDialogs([message]) calls to prevent adding a duplicate message to the conversation history.

src/Infrastructure/BotSharp.Core/Routing/RoutingService.InvokeAgent.cs [177-182]

 message.StopCompletion = true;
 message.Content = "I received a function call request but the function name is missing. Please try again.";
 message.Role = AgentRole.Assistant;
-dialogs.Add(message);
-Context.AddDialogs([message]);
 return true;
Suggestion 4:

Prevent duplicate hook side effects

Replace currently calls Push, which emits OnAgentEnqueued, and then also emits OnAgentReplaced, causing duplicated hook side effects; update Replace to only emit the replacement hook and avoid invoking Push's enqueue logic.

src/Infrastructure/BotSharp.Core/Routing/RoutingContext.cs [199-207]

 else if (_stack.Peek() != agentId)
 {
     fromAgent = _stack.Peek();
     _stack.Pop();
-    await Push(agentId);
+    _stack.Push(agentId);
 
     await HookEmitter.Emit<IRoutingHook>(_services, async hook => await hook.OnAgentReplaced(fromAgent, toAgent, reason: reason),
         agentId);
 }
Suggestion 5:

Log exceptions instead of swallowing

Avoid swallowing exceptions; at minimum log the exception so callers/operators can understand why parsing failed before repair is attempted.

src/Infrastructure/BotSharp.Core/JsonRepair/JsonRepairService.cs [33-41]

 try
 {
     // First try direct deserialization
     return malformedJson.JsonContent<T>();
 }
-catch
+catch (Exception ex)
 {
+    _logger.LogDebug(ex, "Direct JSON deserialization failed; attempting LLM repair.");
     // Continue to repair
 }
Suggestion 6:

Use appropriate log level for failures

When an element cannot be located and it's not explicitly ignored, this represents a failure condition that should be logged as a warning or error. Using LogInformation may make it difficult to identify actual problems during debugging and monitoring.

src/Plugins/BotSharp.Plugin.WebDriver/Drivers/PlaywrightDriver/PlaywrightWebDriver.LocateElement.cs [85-86]

 result.Message = $"Can't locate element by keyword {location.Text}";
-_logger.LogInformation(result.Message);
+_logger.LogWarning(result.Message);
Suggestion 7:

Correct a misleading log message

Correct a duplicated log message that appears both before and after PythonEngine.Exec. The second message should be changed from "Before executing..." to "After executing..." to accurately reflect the execution flow.

src/Plugins/BotSharp.Plugin.PythonInterpreter/Services/PyCodeInterpreter.cs [209-212]

 // Execute Python script
 PythonEngine.Exec(codeScript, globals);
 
-_logger.LogWarning($"Before executing code script {options?.ScriptName}. Thread: {Thread.CurrentThread.ManagedThreadId}.");
+_logger.LogWarning($"After executing code script {options?.ScriptName}. Thread: {Thread.CurrentThread.ManagedThreadId}.");
Suggestion 8: [security] GoogleAI logs response `text`
GoogleAI logs response `text` The new warning logs include the full model output `text` (and related metadata), which may contain sensitive information and should not be logged. The updated MaxTokens fallback message includes the internal max-token value, exposing configuration details to the end user.

Issue description

GoogleAI provider logs raw assistant content (text) and exposes the internal MaxOutputTokens value in a user-facing message.

Issue Context

Logs must not include sensitive content, and user-visible errors should be generic to avoid leaking internal system/config details.

Fix Focus Areas

  • src/Plugins/BotSharp.Plugin.GoogleAI/Providers/Chat/ChatCompletionProvider.cs[75-84]
  • src/Plugins/BotSharp.Plugin.GoogleAI/Providers/Chat/ChatCompletionProvider.cs[184-200]
Suggestion 9: [security] OpenAI logs response `text`
OpenAI logs response `text` The updated warning logs include full model output content via `Content:{text}`, which may expose sensitive data in logs. The new Length handling block also returns a user-facing message that includes the internal max token count.

Issue description

OpenAI provider logs full assistant output (text) in warnings and includes internal token limit values in a user-facing fallback message.

Issue Context

To comply with secure logging and secure error handling, avoid logging sensitive content and avoid exposing internal configuration details to end users.

Fix Focus Areas

  • src/Plugins/BotSharp.Plugin.OpenAI/Providers/Chat/ChatCompletionProvider.cs[76-85]
  • src/Plugins/BotSharp.Plugin.OpenAI/Providers/Chat/ChatCompletionProvider.cs[191-203]

[Auto-generated best practices - 2026-07-03]

Clone this wiki locally