optimize GetAgents

[yileicn]

PR Type

Enhancement

---

Description

• Improve agent ID validation in GetAgent method

• Add empty GUID check and warning log

• Enhance error handling for invalid agent IDs

---

Diagram Walkthrough

flowchart LR
  A["Agent ID Input"] --> B["Enhanced Validation"]
  B --> C["Empty/Null Check"]
  B --> D["Empty GUID Check"]
  C --> E["Warning Log"]
  D --> E
  E --> F["Return Null"]

Loading

File Walkthrough

	Relevant files
Enhancement	AgentService.GetAgents.cs Enhanced agent ID validation logic                                              src/Infrastructure/BotSharp.Core/Agents/Services/AgentService.GetAgents.cs Enhanced agent ID validation to include empty GUID check Added warning log for empty agent IDs Improved error handling in GetAgent method +12/-11

---

[qodo-merge-pro]

PR Reviewer Guide 🔍

Here are some key observations to aid the review process:

⏱️ Estimated effort to review: 2 🔵🔵⚪⚪⚪

🧪 No relevant tests

🔒 No security concerns identified

⚡ Recommended focus areas for review Logging Level Using LogWarning for empty/whitespace/empty-GUID input may be noisy if this is user-driven input; consider LogDebug or structured logging to reduce noise and improve observability. if (string.IsNullOrWhiteSpace(id) || id == Guid.Empty.ToString()) { _logger.LogWarning($"Can't find agent {id}, AgentId is empty."); return null; } Nullability Method signature returns Agent but now returns null in more cases; ensure callers are annotated/updated to handle null and that any caching layer respects null returns with SharpCache. public async Task<Agent> GetAgent(string id) { if (string.IsNullOrWhiteSpace(id) || id == Guid.Empty.ToString()) { _logger.LogWarning($"Can't find agent {id}, AgentId is empty."); return null; } var profile = _db.GetAgent(id); if (profile == null) { _logger.LogError($"Can't find agent {id}"); return null; } Guid Check Robustness Comparing to Guid.Empty.ToString() assumes exact formatting; consider parsing with Guid.TryParse and comparing to Guid.Empty to be culture/format safe. if (string.IsNullOrWhiteSpace(id) || id == Guid.Empty.ToString()) { _logger.LogWarning($"Can't find agent {id}, AgentId is empty."); return null; }

[qodo-merge-pro]

PR Code Suggestions ✨

Explore these optional code suggestions:

Category	Suggestion	Impact
High-level	Avoid logging on cache misses GetAgent is SharpCache-decorated, but it now logs warnings/errors for null/empty IDs and not-found agents on every call, which can create noisy logs and mask real issues when cache lookups or validation happen frequently. Consider returning null without logging for expected validation failures and not-found cases, or downgrade to trace/debug and centralize logs at the call sites where a missing agent is truly exceptional. Examples: src/Infrastructure/BotSharp.Core/Agents/Services/AgentService.GetAgents.cs [47-58] if (string.IsNullOrWhiteSpace(id) || id == Guid.Empty.ToString()) { _logger.LogWarning($"Can't find agent {id}, AgentId is empty."); return null; } var profile = _db.GetAgent(id); if (profile == null) { _logger.LogError($"Can't find agent {id}"); ... (clipped 2 lines) Solution Walkthrough: Before: [SharpCache(10)] public async Task<Agent> GetAgent(string id) { if (string.IsNullOrWhiteSpace(id) || id == Guid.Empty.ToString()) { _logger.LogWarning($"Can't find agent {id}, AgentId is empty."); return null; } var profile = _db.GetAgent(id); if (profile == null) { _logger.LogError($"Can't find agent {id}"); return null; } // ... } After: [SharpCache(10)] public async Task<Agent> GetAgent(string id) { if (string.IsNullOrWhiteSpace(id) || id == Guid.Empty.ToString()) { // Return null without logging. The caller has the context // to decide if this is an exceptional case worth logging. return null; } var profile = _db.GetAgent(id); if (profile == null) { // A cache miss is not necessarily an error. return null; } // ... } Suggestion importance[1-10]: 8 __ Why: This is a strong design suggestion that correctly identifies how logging within a cached method can lead to excessive log noise, proposing a valid alternative that improves system observability.	Medium
Possible issue	Use Guid.TryParse for validation Avoid string comparison for empty GUID detection, as format differences can bypass the check. Parse the GUID and compare against Guid.Empty to ensure robust validation. Also, guard against invalid GUID formats to prevent misleading logs. src/Infrastructure/BotSharp.Core/Agents/Services/AgentService.GetAgents.cs [47-51] -if (string.IsNullOrWhiteSpace(id) || id == Guid.Empty.ToString()) +if (string.IsNullOrWhiteSpace(id) || (Guid.TryParse(id, out var gid) && gid == Guid.Empty)) { - _logger.LogWarning($"Can't find agent {id}, AgentId is empty."); + _logger.LogWarning("Can't find agent {AgentId}, AgentId is empty.", id); return null; } Apply / Chat Suggestion importance[1-10]: 7 __ Why: The suggestion correctly points out that comparing a string to Guid.Empty.ToString() is not robust, and using Guid.TryParse is a better approach to handle different GUID formats.	Medium
General	Prefer structured logging templates Use structured logging with message templates instead of string interpolation for better log parsing and performance. This also avoids issues if logging is disabled at this level. src/Infrastructure/BotSharp.Core/Agents/Services/AgentService.GetAgents.cs [49] -_logger.LogWarning($"Can't find agent {id}, AgentId is empty."); +_logger.LogWarning("Can't find agent {AgentId}, AgentId is empty.", id); Apply / Chat Suggestion importance[1-10]: 5 __ Why: This is a valid best practice suggestion that improves logging by using structured templates instead of string interpolation, which benefits performance and log analysis.	Low
More

[JackJiang1234]

reviewed