refine json repair & graph db

[iceljc]

PR Type

Enhancement, Refactoring

---

Description

• Refactored JSON repair service from plugin to core shared module

• Enhanced graph database interface with async methods and expanded result model

• Introduced IGraphKnowledgeService to separate graph knowledge concerns

• Added JsonRepairOptions for flexible LLM configuration in JSON repair

• Improved GraphSearchResult with keys and values arrays for structured data

• Updated GraphSearchOptions with provider, graph ID, and arguments support

---

Diagram Walkthrough

flowchart LR
  A["JSON Repair Plugin"] -->|Moved to Core| B["JsonRepairService in Shared"]
  C["IKnowledgeService"] -->|Delegates to| D["IGraphKnowledgeService"]
  E["IGraphDb.Search"] -->|Renamed to| F["IGraphDb.SearchAsync"]
  G["GraphSearchData"] -->|Replaced by| H["GraphSearchResult"]
  H -->|Enhanced with| I["Keys and Values arrays"]
  J["GraphSearchOptions"] -->|Extended with| K["Provider, GraphId, Arguments"]

Loading

File Walkthrough

	Relevant files
Enhancement	14 files IGraphDb.cs Renamed Search to SearchAsync with optional parameters      +1/-1      GraphSearchResult.cs Enhanced with keys and values array properties                      +3/-1      GraphSearchOptions.cs Added provider, graph ID, and arguments properties              +4/-1      IGraphKnowledgeService.cs New interface for graph knowledge search operations            +9/-0      IJsonRepairService.cs Renamed methods to async and added options parameter          +7/-3      JsonRepairOptions.cs New options class for JSON repair configuration                    +25/-0    InstructService.Execute.cs Updated to use new RepairAsync method with options              +4/-3      JsonRepairService.cs New implementation with options and flexible LLM config    +130/-0  InstructModeController.cs Added response format parameter to execute call                    +2/-1      KnowledgeBaseController.cs Injected IGraphKnowledgeService and updated search method +7/-1      SearchGraphKnowledgeRequest.cs Added provider, graph ID, and arguments request properties +14/-1    GraphDb.cs Renamed Search to SearchAsync and updated return types      +9/-6      GraphKnowledgeService.cs New service implementing IGraphKnowledgeService interface +45/-0    MembaseGraphDb.cs New Membase graph database implementation with async search +55/-0
Refactoring	8 files GraphSearchData.cs Removed deprecated GraphSearchData model class                      +0/-6      IKnowledgeService.cs Removed graph search method from knowledge service              +0/-4      StringExtensions.cs Removed non-ASCII regex and simplified JSON cleaning          +5/-7      JsonRepairPlugin.cs Removed deprecated JSON repair plugin file                              +0/-19    JsonRepairService.cs Moved to shared module with enhanced configuration              +0/-113  KnowledgeHook.cs Updated to use IGraphKnowledgeService instead of direct service +5/-5      KnowledgeService.Graph.cs Removed graph search implementation from knowledge service +0/-24    KnowledgeService.cs Removed GetGraphDb helper method from knowledge service    +0/-6
Formatting	1 files AgentService.Rendering.cs Improved code formatting and null check handling                  +4/-1
Configuration changes	3 files InsturctionPlugin.cs Registered JsonRepairService in instruction plugin DI        +3/-0      KnowledgeBasePlugin.cs Registered GraphKnowledgeService in dependency injection  +2/-0      BotSharp.Core.csproj Added folder reference and updated project structure          +5/-1

---

[qodo-code-review]

PR Compliance Guide 🔍

Below is a summary of compliance checks for this PR:

Security Compliance
🔴	Sensitive data logging Description: The new JsonRepairService logs full JSON payloads and LLM outputs (e.g., _logger.LogError(ex, $"Error when parse json {malformedJson}") and _logger.LogInformation($"JSON repair result: {response.Content}")), which can exfiltrate sensitive user/agent data into application logs if the malformed JSON or repaired output contains secrets, tokens, PII, or proprietary content. JsonRepairService.cs [58-126] Referred Code private bool IsValidJson(string malformedJson) { if (string.IsNullOrWhiteSpace(malformedJson)) { return false; } try { JsonDocument.Parse(malformedJson); return true; } catch (JsonException ex) { _logger.LogError(ex, $"Error when parse json {malformedJson}"); return false; } } private async Task<string> RepairByLLMAsync(string malformedJson, JsonRepairOptions? options) { ... (clipped 48 lines)
⚪	Cypher query injection Description: MembaseGraphDb.SearchAsync forwards the caller-provided query directly into CypherQueryAsync as an executable Cypher statement, enabling any authorized caller reaching this path to run arbitrary graph queries (potentially including writes or data exfiltration) unless upstream authorization/allowlisting restricts permitted operations. MembaseGraphDb.cs [27-48] Referred Code public async Task<GraphSearchResult> SearchAsync(string query, GraphSearchOptions? options = null) { if (string.IsNullOrEmpty(options?.GraphId)) { throw new ArgumentException($"Please provide a valid {Provider} graph id."); } try { var response = await _membaseApi.CypherQueryAsync(options.GraphId, new CypherQueryRequest { Query = query, Parameters = options.Arguments ?? [] }); return new GraphSearchResult { Keys = response.Columns, Values = response.Data, Result = JsonSerializer.Serialize(response.Data) }; ... (clipped 1 lines)
	Provider selection abuse Description: GraphKnowledgeService.GetGraphDb selects an IGraphDb implementation using options?.Provider (which is populated from API input), which can allow a caller to route requests to unintended graph backends (cross-tenant or higher-privilege providers) if DI-registered providers are not access-controlled per user/tenant. GraphKnowledgeService.cs [38-43] Referred Code private IGraphDb GetGraphDb(string? provider = null) { var graphProvider = provider ?? _settings.GraphDb.Provider; var db = _services.GetServices<IGraphDb>().FirstOrDefault(x => x.Provider == graphProvider); return db; }
Ticket Compliance
⚪	🎫 No ticket provided Create ticket/issue
Codebase Duplication Compliance
⚪	Codebase context is not defined Follow the guide to enable codebase context checks.
Custom Compliance
🟢	Generic: Meaningful Naming and Self-Documenting Code Objective: Ensure all identifiers clearly express their purpose and intent, making code self-documenting Status: Passed Learn more about managing compliance generic rules or creating your own custom rules
🔴	Generic: Robust Error Handling and Edge Case Management Objective: Ensure comprehensive error handling that provides meaningful context and graceful degradation Status: Null provider handling: GetGraphDb may return null when no matching IGraphDb is registered, and SearchAsync then calls db.SearchAsync(...) without a null-check leading to a potential null reference at runtime. Referred Code private IGraphDb GetGraphDb(string? provider = null) { var graphProvider = provider ?? _settings.GraphDb.Provider; var db = _services.GetServices<IGraphDb>().FirstOrDefault(x => x.Provider == graphProvider); return db; } Learn more about managing compliance generic rules or creating your own custom rules
	Generic: Secure Error Handling Objective: To prevent the leakage of sensitive system information through error messages while providing sufficient detail for internal debugging. Status: Leaky exception message: Throwing ArgumentException($"Please provide a valid {Provider} graph id.") can surface internal provider details in user-facing errors rather than returning a generic error and logging details internally. Referred Code if (string.IsNullOrEmpty(options?.GraphId)) { throw new ArgumentException($"Please provide a valid {Provider} graph id."); } Learn more about managing compliance generic rules or creating your own custom rules
	Generic: Secure Logging Practices Objective: To ensure logs are useful for debugging and auditing without exposing sensitive information like PII, PHI, or cardholder data. Status: Logs raw JSON: The service logs raw JSON input and LLM output (e.g., Error when parse json {malformedJson} and JSON repair result: {response.Content}) which can leak sensitive data into logs. Referred Code catch (JsonException ex) { _logger.LogError(ex, $"Error when parse json {malformedJson}"); return false; } } private async Task<string> RepairByLLMAsync(string malformedJson, JsonRepairOptions? options) { var agentId = options?.AgentId ?? BuiltInAgentId.AIAssistant; var templateName = options?.TemplateName ?? DEFAULT_TEMPLATE_NAME; var agentService = _services.GetRequiredService<IAgentService>(); var agent = await agentService.GetAgent(agentId); var template = agent?.Templates?.FirstOrDefault(x => x.Name == templateName)?.Content; if (string.IsNullOrEmpty(template)) { _logger.LogWarning($"Template '{templateName}' cannot be found in agent '{agent?.Name ?? agentId}'"); return malformedJson; } ... (clipped 32 lines) Learn more about managing compliance generic rules or creating your own custom rules
⚪
	Update

Compliance status legend

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

[qodo-code-review]

PR Code Suggestions ✨

Latest suggestions up to 2a7ac73

Category	Suggestion	Impact
Possible issue	Handle missing provider safely In ExecuteGraphQuery, replace First() with FirstOrDefault() and add a null check to handle cases where the 'membase' graph provider is not registered, preventing a potential server error. src/Plugins/BotSharp.Plugin.Membase/Controllers/MembaseController.cs [42-52] -var graph = _services.GetServices<IGraphDb>().First(x => x.Provider == "membase"); +var graph = _services.GetServices<IGraphDb>().FirstOrDefault(x => x.Provider == "membase"); +if (graph == null) +{ + return StatusCode( + StatusCodes.Status500InternalServerError, + new { message = "Graph provider 'membase' is not registered." }); +} + var result = await graph.SearchAsync(query: request.Query, options: new() { GraphId = graphId, Arguments = request.Parameters }); + return Ok(new { Columns = result.Keys, Items = result.Values }); Apply / Chat Suggestion importance[1-10]: 8 __ Why: The suggestion correctly identifies that using First() can cause an unhandled exception if the provider is not found, and proposes using FirstOrDefault() with a null check for robust error handling.	Medium
	Prevent runtime not-implemented crashes Remove default implementations from the IGraphDb interface to force all implementing classes to provide their own implementation, thus preventing potential runtime NotImplementedException errors. src/Infrastructure/BotSharp.Abstraction/Graph/IGraphDb.cs [8-27] public interface IGraphDb { public string Provider { get; } - Task<GraphSearchResult> SearchAsync(string query, GraphSearchOptions? options = null) - => throw new NotImplementedException(); + Task<GraphSearchResult> SearchAsync(string query, GraphSearchOptions? options = null); #region Node - Task<GraphNode?> GetNodeAsync(string graphId, string nodeId) - => throw new NotImplementedException(); - Task<GraphNode> CreateNodeAsync(string graphId, GraphNodeCreationModel node) - => throw new NotImplementedException(); - Task<GraphNode> UpdateNodeAsync(string graphId, string nodeId, GraphNodeUpdateModel node) - => throw new NotImplementedException(); - Task<GraphNode> UpsertNodeAsync(string graphId, string nodeId, GraphNodeUpdateModel node) - => throw new NotImplementedException(); - Task<GraphNodeDeleteResponse?> DeleteNodeAsync(string graphId, string nodeId) - => throw new NotImplementedException(); + Task<GraphNode?> GetNodeAsync(string graphId, string nodeId); + Task<GraphNode> CreateNodeAsync(string graphId, GraphNodeCreationModel node); + Task<GraphNode> UpdateNodeAsync(string graphId, string nodeId, GraphNodeUpdateModel node); + Task<GraphNode> UpsertNodeAsync(string graphId, string nodeId, GraphNodeUpdateModel node); + Task<GraphNodeDeleteResponse?> DeleteNodeAsync(string graphId, string nodeId); #endregion } Apply / Chat Suggestion importance[1-10]: 7 __ Why: The suggestion correctly points out that default interface implementations can hide incomplete implementations, leading to runtime errors, and proposes a safer design that enforces implementation at compile time.	Medium
Security	Reduce sensitive log exposure In IsValidJson, avoid logging the full malformedJson string on parsing failure to prevent exposing sensitive data. Instead, log only a truncated preview and its length. src/Infrastructure/BotSharp.Core/Shared/JsonRepairService.cs [70-74] catch (JsonException ex) { - _logger.LogError(ex, $"Error when parse json {malformedJson}"); + var preview = malformedJson.Length <= 512 + ? malformedJson + : malformedJson.Substring(0, 512); + + _logger.LogError(ex, "Error parsing json (len={Len}, preview={Preview})", malformedJson.Length, preview); return false; } Apply / Chat Suggestion importance[1-10]: 8 __ Why: This is a valid security and performance suggestion, as logging the full JSON string can expose sensitive data and cause log bloat; the proposed change mitigates this risk.	Medium
Incremental [*]	Avoid mutating caller-provided data In RepairByLLMAsync, avoid mutating the options.Data dictionary directly. Instead, create a new dictionary, copy the data from options.Data, and then add the input key to it. src/Infrastructure/BotSharp.Core/Shared/JsonRepairService.cs [92-95] var render = _services.GetRequiredService<ITemplateRender>(); -var data = options?.Data ?? new Dictionary<string, object>(); -data["input"] = malformedJson; +var data = new Dictionary<string, object>(options?.Data ?? new Dictionary<string, object>()) +{ + ["input"] = malformedJson +}; var prompt = render.Render(template, data); Apply / Chat Suggestion importance[1-10]: 7 __ Why: The suggestion correctly identifies a potential side effect of mutating a shared dictionary. Creating a defensive copy prevents state leakage and hard-to-debug issues, making the code more robust.	Medium
More

---

Previous suggestions

✅ Suggestions up to commit 2fc121e

Category	Suggestion	Impact
Possible issue	✅ Use empty Dictionary as default Suggestion Impact: The commit changes the fallback initialization from [] to new Dictionary(), matching the suggested fix to avoid an invalid empty collection literal. code diff: - var data = options?.Data ?? []; + var data = options?.Data ?? new Dictionary<string, object>(); data["input"] = malformedJson; Replace the invalid empty collection literal [] with new Dictionary<string, object>() to correctly initialize the default data dictionary. src/Infrastructure/BotSharp.Core/Shared/JsonRepairService.cs [93-94] -var data = options?.Data ?? []; +var data = options?.Data ?? new Dictionary<string, object>(); data["input"] = malformedJson; [Suggestion processed] Suggestion importance[1-10]: 9 __ Why: The suggestion correctly identifies a compilation error where an invalid empty collection literal [] is used as a fallback for a Dictionary<string, object>, and provides the correct fix.	High
	✅ Fix default arguments dictionary Suggestion Impact: The specific problematic line (`Parameters = options.Arguments ?? []`) was removed because the entire `MembaseGraphDb.cs` implementation was deleted, which indirectly eliminates the compilation issue, but the suggested replacement was not implemented. code diff: - var response = await _membaseApi.CypherQueryAsync(options.GraphId, new CypherQueryRequest - { - Query = query, - Parameters = options.Arguments ?? [] - }); Replace the invalid empty collection literal [] with new Dictionary<string, object?>() to correctly initialize the default Parameters dictionary. src/Plugins/BotSharp.Plugin.Membase/Services/MembaseGraphDb.cs [36-40] var response = await _membaseApi.CypherQueryAsync(options.GraphId, new CypherQueryRequest { Query = query, - Parameters = options.Arguments ?? [] + Parameters = options.Arguments ?? new Dictionary<string, object?>() }); [Suggestion processed] Suggestion importance[1-10]: 9 __ Why: The suggestion correctly identifies a compilation error where an invalid empty collection literal [] is used as a fallback for a Dictionary<string, object?>, and provides the correct fix.	High
High-level	Expose structured graph search results The SearchGraphKnowledge API endpoint should be updated to return the new structured Keys and Values fields from GraphSearchResult, not just the raw Result string, to provide richer data to consumers. Examples: src/Infrastructure/BotSharp.OpenAPI/Controllers/KnowledgeBase/KnowledgeBaseController.cs [219-222] return new GraphKnowledgeViewModel { Result = result.Result }; src/Infrastructure/BotSharp.Abstraction/Graph/Models/GraphSearchResult.cs [3-8] public class GraphSearchResult { public string Result { get; set; } = string.Empty; public string[] Keys { get; set; } = []; public Dictionary<string, object?>[] Values { get; set; } = []; } Solution Walkthrough: Before: // In KnowledgeBaseController.cs public async Task<GraphKnowledgeViewModel> SearchGraphKnowledge(...) { // ... var result = await _graphKnowledgeService.SearchAsync(request.Query, options); return new GraphKnowledgeViewModel { Result = result.Result // Only the raw string is returned }; } // Inferred GraphKnowledgeViewModel public class GraphKnowledgeViewModel { public string Result { get; set; } } After: // In KnowledgeBaseController.cs public async Task<GraphKnowledgeViewModel> SearchGraphKnowledge(...) { // ... var result = await _graphKnowledgeService.SearchAsync(request.Query, options); return new GraphKnowledgeViewModel { Result = result.Result, Keys = result.Keys, Values = result.Values }; } // Modified GraphKnowledgeViewModel public class GraphKnowledgeViewModel { public string Result { get; set; } public string[] Keys { get; set; } public Dictionary<string, object?>[] Values { get; set; } } Suggestion importance[1-10]: 8 __ Why: The suggestion correctly identifies that the new structured graph search results (Keys and Values) are discarded in the KnowledgeBaseController, preventing API consumers from accessing this valuable data.	Medium
General	Guard against missing graph provider Add a null check after retrieving the IGraphDb service to prevent a NullReferenceException if no matching provider is found, throwing an InvalidOperationException instead. src/Plugins/BotSharp.Plugin.KnowledgeBase/Graph/GraphKnowledgeService.cs [38-43] private IGraphDb GetGraphDb(string? provider = null) { var graphProvider = provider ?? _settings.GraphDb.Provider; - var db = _services.GetServices<IGraphDb>().FirstOrDefault(x => x.Provider == graphProvider); + var db = _services.GetServices<IGraphDb>() + .FirstOrDefault(x => x.Provider == graphProvider); + if (db == null) + { + throw new InvalidOperationException($"No IGraphDb registered for provider '{graphProvider}'."); + } return db; } Suggestion importance[1-10]: 8 __ Why: The suggestion correctly identifies a potential NullReferenceException if no IGraphDb provider is found and proposes a robust solution by adding a null check and throwing an explicit exception.	Medium
Learned best practice	✅ Copy mutable dictionaries before mutation Suggestion Impact: The commit modified the same line to avoid using the empty collection literal by initializing with a new Dictionary when options.Data is null, but it did not implement the key part of the suggestion (copying options.Data when it is non-null before mutation). code diff: var render = _services.GetRequiredService<ITemplateRender>(); - var data = options?.Data ?? []; + var data = options?.Data ?? new Dictionary<string, object>(); data["input"] = malformedJson; 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 processed] Suggestion importance[1-10]: 6 __ Why: Relevant best practice - When transferring metadata or other mutable collections between objects, copy the dictionary instead of reusing the same reference to avoid shared-state side effects.	Low
	Return safe defaults for missing inputs Don’t throw on missing GraphId for a search call; log and return an empty GraphSearchResult so callers don’t crash on missing/invalid inputs. src/Plugins/BotSharp.Plugin.Membase/Services/MembaseGraphDb.cs [29-32] if (string.IsNullOrEmpty(options?.GraphId)) { - throw new ArgumentException($"Please provide a valid {Provider} graph id."); + _logger.LogWarning("Missing graph id for {Provider} graph db query.", Provider); + return new GraphSearchResult(); } Suggestion importance[1-10]: 5 __ Why: Relevant best practice - Add explicit null/empty guards and return a safe default instead of throwing when inputs are missing.	Low