.pr_agent_auto_best_practices

Pattern 1: When exposing, storing, or mutating collections/dictionaries that originate from callers, caches, or provider responses, create defensive copies (including copying contained objects when needed) to avoid shared mutable state across requests.

Example code before:

public void AddEdge(Payload payload)
{
    _edges.Add(new Edge
    {
        Labels = payload.Labels,     // shared ref
        Config = payload.Config      // shared ref
    });
}

public GraphInfo GetInfo() => new() { Nodes = _nodes, Edges = _edges }; // leaks internal lists

Example code after:

public void AddEdge(Payload payload)
{
    _edges.Add(new Edge
    {
        Labels = payload.Labels?.ToList(),
        Config = payload.Config is null ? null : new Dictionary<string, object>(payload.Config)
    });
}

public GraphInfo GetInfo() => new()
{
    Nodes = _nodes.ToList(),
    Edges = _edges.ToList()
};

Relevant past accepted suggestions:

Suggestion 1:

[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 2:

[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 3:

[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 4:

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 5:

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 6:

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 guards at API and integration boundaries (request bodies, provider responses, parsed data, and filtered results), and return safe fallbacks rather than throwing due to null dereferences or empty sequences.

Example code before:

public string Handle(Request req)
{
    var bytes = Convert.FromBase64String(req.Payload); // req may be null
    return Process(bytes);
}

var choice = models.ElementAt(new Random().Next(models.Count())); // may be empty

Example code after:

public string Handle(Request? req)
{
    if (string.IsNullOrWhiteSpace(req?.Payload)) return string.Empty;
    var bytes = Convert.FromBase64String(req.Payload);
    return Process(bytes);
}

var available = models.ToList();
if (available.Count == 0) return null;
var choice = available[new Random().Next(available.Count)];

Relevant past accepted suggestions:

Suggestion 1:

[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 2:

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 3:

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 4:

Fix null reference bug

The condition has a logical error. It should use AND (&&) instead of OR (||) to ensure both that args is not null and the delay time is positive. The current condition will attempt to access DelayTime even when args is null, causing a NullReferenceException.

src/Infrastructure/BotSharp.Core.Crontab/Functions/TaskWaitFn.cs [25]

-if (args != null || args.DelayTime > 0)
+if (args != null && args.DelayTime > 0)

Suggestion 5:

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);
+    }
 }

Suggestion 6:

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;

Pattern 3: For background/async execution and external I/O, ensure failures are observable and cleanup always runs: log exceptions in fire-and-forget tasks, avoid sync-over-async deadlocks, correctly release locks/semaphores, and handle cancellation without skipping finally blocks.

Example code before:

Task.Run(() => DoWork()); // exceptions can be lost

_sem.Wait(ct);
try { return Run(); }
finally { _sem.Release(); } // may release without acquire if Wait throws

var stdout = await proc.StandardOutput.ReadToEndAsync();
await proc.WaitForExitAsync(); // can deadlock if buffers fill

Example code after:

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

var acquired = false;
try { _sem.Wait(ct); acquired = true; return Run(); }
finally { if (acquired) _sem.Release(); }

var stdoutTask = proc.StandardOutput.ReadToEndAsync(ct);
var stderrTask = proc.StandardError.ReadToEndAsync(ct);
await Task.WhenAll(proc.WaitForExitAsync(ct), stdoutTask, stderrTask);

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:

Prevent potential deadlock in semaphore usage

To prevent potential deadlocks, revert the InnerRunWithLock method to use a boolean flag to ensure the semaphore is always released correctly, even in the case of asynchronous exceptions.

src/Plugins/BotSharp.Plugin.PythonInterpreter/Services/PyCodeInterpreter.cs [103-120]

 private CodeInterpretResponse InnerRunWithLock(string codeScript, CodeInterpretOptions? options = null, CancellationToken cancellationToken = default)
 {
-    _semLock.Wait(cancellationToken);
-
+    var lockAcquired = false;
     try
     {
+        _semLock.Wait(cancellationToken);
+        lockAcquired = true;
         return InnerRunCode(codeScript, options, cancellationToken);
     }
     catch (Exception ex)
     {
         _logger.LogError(ex, $"Error in {nameof(InnerRunWithLock)}");
         return new() { ErrorMsg = ex.Message };
     }
     finally
     {
-        _semLock.Release();
+        if (lockAcquired)
+        {
+            _semLock.Release();
+        }
     }
 }

Suggestion 3:

Ensure cleanup on cancellation

Refactor the task cancellation logic to ensure the finally block always executes, preventing the Python interpreter's state from being left corrupted upon cancellation. This involves removing the cancellation token from Task.Run and handling cancellation exceptions from WaitAsync to allow for cleanup.

src/Plugins/BotSharp.Plugin.PythonInterpreter/Services/PyCodeInterpreter.cs [137-216]

 private async Task<CodeInterpretResponse> CoreRunScript(string codeScript, CodeInterpretOptions? options = null, CancellationToken cancellationToken = default)
 {
     cancellationToken.ThrowIfCancellationRequested();
 
     var execTask = Task.Run(() =>
     {
         using (Py.GIL())
         {
-            // Import necessary Python modules
             dynamic sys = Py.Import("sys");
             dynamic io = Py.Import("io");
 
             try
             {
-                // Redirect standard output/error to capture it
                 dynamic outIO = io.StringIO();
                 dynamic errIO = io.StringIO();
                 sys.stdout = outIO;
                 sys.stderr = errIO;
 
-                // Set global items
                 using var globals = new PyDict();
                 if (codeScript.Contains("__main__") == true)
                 {
                     globals.SetItem("__name__", new PyString("__main__"));
                 }
 
-                // Set arguments
                 var list = new PyList();
                 if (options?.Arguments?.Any() == true)
                 {
                     list.Append(new PyString(options?.ScriptName ?? "script.py"));
 
                     foreach (var arg in options!.Arguments)
                     {
                         if (!string.IsNullOrWhiteSpace(arg.Key) && !string.IsNullOrWhiteSpace(arg.Value))
                         {
                             list.Append(new PyString($"--{arg.Key}"));
                             list.Append(new PyString($"{arg.Value}"));
                         }
                     }
                 }
                 sys.argv = list;
 
-                cancellationToken.ThrowIfCancellationRequested();
+                // cooperative cancellation point before exec
+                if (cancellationToken.IsCancellationRequested)
+                {
+                    cancellationToken.ThrowIfCancellationRequested();
+                }
 
-                // Execute Python script
                 PythonEngine.Exec(codeScript, globals);
 
-                // Get result
                 var stdout = outIO.getvalue()?.ToString() as string;
-                var stderr = errIO.getvalue()?.ToString() as string;
 
-                cancellationToken.ThrowIfCancellationRequested();
+                // cooperative cancellation after exec
+                if (cancellationToken.IsCancellationRequested)
+                {
+                    cancellationToken.ThrowIfCancellationRequested();
+                }
 
                 return new CodeInterpretResponse
                 {
                     Result = stdout?.TrimEnd('\r', '\n') ?? string.Empty,
                     Success = true
                 };
             }
             catch (Exception ex)
             {
                 _logger.LogError(ex, $"Error in {nameof(CoreRunScript)} in {Provider}.");
                 return new() { ErrorMsg = ex.Message };
             }
             finally
             {
-                // Restore the original stdout/stderr/argv
                 sys.stdout = sys.__stdout__;
                 sys.stderr = sys.__stderr__;
                 sys.argv = new PyList();
             }
-        };
-    }, cancellationToken);
+        }
+    }); // do not pass cancellationToken here
 
-    return await execTask.WaitAsync(cancellationToken);
+    try
+    {
+        return await execTask.WaitAsync(cancellationToken);
+    }
+    catch (OperationCanceledException)
+    {
+        // Ensure the inner task completes cleanup
+        try { await execTask; } catch { /* ignored: already logged */ }
+        throw;
+    }
 }

Suggestion 4:

Prevent potential process execution deadlock

Prevent a potential process deadlock by reading the standard output and error streams concurrently with waiting for the process to exit using Task.WhenAll.

src/Plugins/BotSharp.Plugin.PythonInterpreter/Services/PyCodeInterpreter.cs [267-273]

-var stdoutTask = proc.StandardOutput.ReadToEndAsync();
-var stderrTask = proc.StandardError.ReadToEndAsync();
+var stdoutTask = proc.StandardOutput.ReadToEndAsync(token);
+var stderrTask = proc.StandardError.ReadToEndAsync(token);
 
-await proc.WaitForExitAsync();
+await Task.WhenAll(proc.WaitForExitAsync(token), stdoutTask, stderrTask);
 
 var stdout = await stdoutTask;
 var stderr = await stderrTask;

Suggestion 5:

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 6:

Fix WebSocket fragmentation handling

The code is using a fixed-size buffer for receiving WebSocket messages, but it doesn't handle fragmented messages correctly. WebSocket messages can be split across multiple frames, and the current implementation will process each frame independently, potentially causing data corruption for large messages.

src/Plugins/BotSharp.Plugin.ChatHub/ChatStreamMiddleware.cs [61-77]

 var buffer = new byte[1024 * 32];
 WebSocketReceiveResult result;
+var messageBuffer = new List<byte>();
 
 do
 {
     result = await webSocket.ReceiveAsync(new(buffer), CancellationToken.None);
 
     if (result.MessageType != WebSocketMessageType.Text)
     {
         continue;
     }
 
-    var receivedText = Encoding.UTF8.GetString(buffer, 0, result.Count);
-    if (string.IsNullOrEmpty(receivedText))
+    messageBuffer.AddRange(new ArraySegment<byte>(buffer, 0, result.Count));
+    
+    if (result.EndOfMessage)
     {
-        continue;
-    }
+        var receivedText = Encoding.UTF8.GetString(messageBuffer.ToArray());
+        messageBuffer.Clear();
+        
+        if (string.IsNullOrEmpty(receivedText))
+        {
+            continue;
+        }

Suggestion 7:

Avoid potential deadlocks

The code is using .Result to synchronously wait for an asynchronous operation, which can lead to deadlocks in ASP.NET applications. This is a common anti-pattern that should be avoided.

src/Infrastructure/BotSharp.Core/Routing/RoutingContext.cs [85-93]

 // Convert id to name
 if (!Guid.TryParse(agentId, out _))
 {
     var agentService = _services.GetRequiredService<IAgentService>();
-    var agents = agentService.GetAgentOptions([agentId]).Result;
+    var agents = agentService.GetAgentOptions([agentId]).GetAwaiter().GetResult();
 
     if (agents.Count > 0)
     {
         agentId = agents.First().Id;

[Suggestion has been applied]

Pattern 4: Keep contracts consistent across configuration, persistence, and serialization by ensuring new/renamed fields are mapped end-to-end (domain ↔ storage ↔ API JSON) and by failing fast on misconfiguration instead of silently using defaults.

Example code before:

// Domain added NewFlag, but persistence mapping didn't.
public Doc ToDoc(Domain d) => new() { Id = d.Id /* missing NewFlag */ };

var dbName = string.IsNullOrEmpty(url.Db) ? "DefaultDb" : url.Db; // silent fallback

Example code after:

public Doc ToDoc(Domain d) => new() { Id = d.Id, NewFlag = d.NewFlag };
public Domain ToDomain(Doc doc) => new() { Id = doc.Id, NewFlag = doc.NewFlag };

if (string.IsNullOrWhiteSpace(connString))
    throw new InvalidOperationException("Mongo connection string is required when Mongo storage is enabled.");

Relevant past accepted suggestions:

Suggestion 1:

[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 2:

[correctness] Rules config section mismatch

Rules config section mismatch

Configuration adds a `Rules` section in `appsettings.json`, but `RulesPlugin` binds settings from `Rule`, creating contract drift and likely leaving settings unbound. This breaks the single source of truth and consistent configuration matching requirement.

Issue description

The configuration section name in appsettings.json (Rules) does not match the section name used by DI binding (Rule), causing settings to be unbound or inconsistent.

Issue Context

Compliance requires configuration/contracts to be consistent and robustly matched.

Fix Focus Areas

src/WebStarter/appsettings.json[739-745]
src/Infrastructure/BotSharp.Core.Rules/RulesPlugin.cs[22-25]

Suggestion 3:

[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 4:

[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 5:

[correctness] Tenant connstring overridden

Tenant connstring overridden

TenantConnectionProvider now prefers app-level connection strings over tenant-resolved ones, which can prevent per-tenant connection overrides from taking effect when a global connection string exists. This undermines multi-database multi-tenancy setups.

Issue description

TenantConnectionProvider now returns the app-level connection string first, preventing tenant-specific connection strings from overriding global configuration.

Issue Context

Existing downstream code uses ITenantConnectionProvider to override database connection settings in multi-tenant mode.

Fix Focus Areas

src/Plugins/BotSharp.Plugin.MultiTenancy/MultiTenancy/TenantConnectionProvider.cs[17-25]

Suggested changes

Prefer tenant resolver result first:
var cs = _resolver.GetConnectionString(name); if (!string.IsNullOrWhiteSpace(cs)) return cs;
then fallback to _configuration.GetConnectionString(name).
If the new precedence is intentional, add a configuration flag and update callers/docs accordingly.

Suggestion 6:

Align metadata JSON field name

Add the [JsonPropertyName("function_data")] attribute to the FunctionMetaData property in RoleDialogModel to ensure consistent JSON serialization with DialogMetaData.

src/Infrastructure/BotSharp.Abstraction/Conversations/Models/RoleDialogModel.cs [74-75]

 [JsonIgnore(Condition = JsonIgnoreCondition.WhenWritingNull)]
+[JsonPropertyName("function_data")]
 public Dictionary<string, string?>? FunctionMetaData { get; set; }

Suggestion 7:

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: Treat identifiers (model IDs, function names, triggers) as ordinal identifiers: use Ordinal/OrdinalIgnoreCase comparisons and correctly constructed sets/dictionaries, avoiding culture-sensitive comparisons and comparer misuse.

Example code before:

if (id.Equals(filterId, StringComparison.InvariantCultureIgnoreCase)) { ... }

var set = new HashSet<string>(words);
if (set.Contains(name, StringComparer.OrdinalIgnoreCase)) { ... } // invalid API usage

Example code after:

if (id.Equals(filterId, StringComparison.OrdinalIgnoreCase)) { ... }

var set = new HashSet<string>(words, StringComparer.OrdinalIgnoreCase);
if (!string.IsNullOrEmpty(name) && set.Contains(name)) { ... }

Relevant past accepted suggestions:

Suggestion 1:

[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 2:

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 3:

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;
 }

Suggestion 4:

Correct reasoning capability gating

Decouple reasoning-effort applicability from the temperature map. Using _defaultTemperature.ContainsKey(_model) to gate reasoning will disable it for supported models not listed. Introduce a separate check (e.g., a set of reasoning-capable models) or remove the gate to honor provided levels when the SDK accepts it.

src/Plugins/BotSharp.Plugin.OpenAI/Providers/Chat/ChatCompletionProvider.cs [18-541]

-private readonly Dictionary<string, float> _defaultTemperature = new()
+private readonly HashSet<string> _reasoningCapableModels = new(StringComparer.OrdinalIgnoreCase)
 {
-    { "o3", 1.0f },
-    { "o3-mini", 1.0f },
-    { "o4-mini", 1.0f },
-    { "gpt-5", 1.0f },
-    { "gpt-5-mini", 1.0f },
-    { "gpt-5-nano", 1.0f }
+    "o3", "o3-mini", "o4-mini", "gpt-5", "gpt-5-mini", "gpt-5-nano"
 };
 ...
 private ChatReasoningEffortLevel? ParseReasoningEffortLevel(string? level)
 {
-    if (string.IsNullOrWhiteSpace(level) || !_defaultTemperature.ContainsKey(_model))
+    if (string.IsNullOrWhiteSpace(level))
     {
         return null;
     }
-    var effortLevel = ChatReasoningEffortLevel.Low;
-    switch (level.ToLower())
+    if (!_reasoningCapableModels.Contains(_model))
     {
-        case "medium":
-            effortLevel = ChatReasoningEffortLevel.Medium;
-            break;
-        case "high":
-            effortLevel = ChatReasoningEffortLevel.High;
-            break;
-        default:
-            break;
+        return null;
     }
-    return effortLevel;
+    return level.ToLower() switch
+    {
+        "medium" => ChatReasoningEffortLevel.Medium,
+        "high" => ChatReasoningEffortLevel.High,
+        _ => ChatReasoningEffortLevel.Low
+    };
 }

[Auto-generated best practices - 2026-04-16]

Uh oh!

.pr_agent_auto_best_practices

Issue description

Issue Context

Fix Focus Areas

Issue description

Issue Context

Fix Focus Areas

Issue description

Issue Context

Fix Focus Areas

Issue description

Issue Context

Fix Focus Areas

Issue description

Issue Context

Fix Focus Areas

Implementation notes

Issue description

Issue Context

Fix Focus Areas

Implementation notes

Issue description

Issue Context

Fix Focus Areas

Issue description

Issue Context

Fix Focus Areas

Issue description

Issue Context

Fix Focus Areas

Issue description

Issue Context

Fix Focus Areas

Suggested changes

Issue description

Issue Context

Fix Focus Areas

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Clone this wiki locally