refine repository and coding util #1273

iceljc · 2026-01-07T23:27:08Z

PR Type

Enhancement, Refactoring

Description

Extract code execution config logic into reusable utility class
Add edge management support to Membase graph API
Implement custom authentication handler for Membase API
Add embedding support to node creation and update models
Clean up code formatting and remove unused imports

Diagram Walkthrough

flowchart LR
  A["Code Execution Config"] -->|Extract to Utility| B["CodingUtil.GetCodeExecutionConfig"]
  B -->|Used by| C["RuleEngine"]
  B -->|Used by| D["InstructService"]
  B -->|Used by| E["PyProgrammerFn"]
  F["Membase API"] -->|Add Edge Operations| G["Edge CRUD"]
  F -->|Add Graph Info| H["GraphInfo"]
  I["MembaseAuthHandler"] -->|Centralized Auth| F
  J["Node Model"] -->|Add Embedding| K["EmbeddingInfo"]

File Walkthrough

Relevant files

Refactoring

4 files

CodingUtil.cs `Extract code execution config utility method`	+21/-0
RuleEngine.cs `Use centralized CodingUtil for config retrieval`	+2/-13
InstructService.Execute.cs `Use centralized CodingUtil for config retrieval`	+2/-17
PyProgrammerFn.cs `Use centralized CodingUtil for config retrieval`	+2/-15

Formatting

5 files

CyperGraphModels.cs `Remove blank lines for code formatting`	+0/-3
MembaseController.cs `Remove unused import statement`	+0/-1
CypherQueryRequest.cs `Remove blank lines for code formatting`	+0/-3
CypherQueryResponse.cs `Remove blank line for code formatting`	+0/-1
MembaseService.cs `Clean up unused imports`	+0/-3

Enhancement

12 files

MembasePlugin.cs `Refactor DI registration with auth handler`	+10/-10
EdgeCreationModel.cs `Add edge creation model for graph operations`	+12/-0
EdgeUpdateModel.cs `Add edge update model for graph operations`	+6/-0
NodeCreationModel.cs `Add embedding support to node creation`	+8/-0
NodeUpdateModel.cs `Add embedding support to node updates`	+2/-2
Edge.cs `Add edge response model for graph operations`	+15/-0
EdgeDeleteResponse.cs `Add edge delete response model`	+6/-0
GraphInfo.cs `Add graph information response model`	+18/-0
Node.cs `Add embedding support and clean up imports`	+1/-10
NodeDeleteResponse.cs `Add node delete response model`	+6/-0
IMembaseApi.cs `Add edge CRUD and graph info endpoints`	+24/-7
MembaseAuthHandler.cs `Add custom HTTP message handler for authentication`	+32/-0

Configuration changes

1 files

Using.cs `Add global using for Membase models`	+1/-0

…atures/refine-membase

qodo-code-review · 2026-01-07T23:27:55Z

PR Compliance Guide 🔍

Below is a summary of compliance checks for this PR:

Security Compliance
⚪	Sensitive data logging Description: The handler reads and logs the full raw HTTP response body (`rawResponse`) in `#if DEBUG`, which may expose sensitive data (e.g., user/graph content or secrets returned by the API) into application logs if debug builds are deployed or logs are collected in shared environments. MembaseAuthHandler.cs [20-28] Referred Code protected override async Task<HttpResponseMessage> SendAsync(HttpRequestMessage requestMessage, CancellationToken cancellationToken) { requestMessage.Headers.TryAddWithoutValidation("Authorization", $"Bearer {_settings.ApiKey}"); var response = await base.SendAsync(requestMessage, cancellationToken).ConfigureAwait(false); #if DEBUG var rawResponse = await response.Content.ReadAsStringAsync(); _logger.LogDebug(rawResponse); #endif
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: Secure Error Handling Objective: To prevent the leakage of sensitive system information through error messages while providing sufficient detail for internal debugging. Status: Passed Learn more about managing compliance generic rules or creating your own custom rules
🔴	Generic: Meaningful Naming and Self-Documenting Code Objective: Ensure all identifiers clearly express their purpose and intent, making code self-documenting Status: Ambiguous tuple return: `GetCodeExecutionConfig` returns an unnamed `(bool, bool, int)` tuple which is not self-documenting and can easily lead to misuse at call sites. Referred Code public static (bool, bool, int) GetCodeExecutionConfig(CodingSettings settings, int defaultTimeoutSeconds = 3) { var codeExecution = settings.CodeExecution; var useLock = codeExecution?.UseLock ?? false; var useProcess = codeExecution?.UseProcess ?? false; var timeoutSeconds = codeExecution?.TimeoutSeconds > 0 ? codeExecution.TimeoutSeconds : defaultTimeoutSeconds; return (useLock, useProcess, timeoutSeconds); 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 HTTP body: The handler logs `rawResponse` directly (`_logger.LogDebug(rawResponse)`), which is unstructured and may expose sensitive data returned by the Membase API. Referred Code var rawResponse = await response.Content.ReadAsStringAsync(); _logger.LogDebug(rawResponse); #endif Learn more about managing compliance generic rules or creating your own custom rules
⚪	Generic: Comprehensive Audit Trails Objective: To create a detailed and reliable record of critical system actions for security analysis and compliance. Status: Missing audit logging: New Membase graph capabilities (node/edge CRUD and graph info retrieval) are introduced without any visible audit logging that records actor/context/outcome for these critical actions. Referred Code [Get("/graph/{graphId}")] Task<GraphInfo> GetGraphInfoAsync(string graphId); [Post("/cypher/execute")] Task<CypherQueryResponse> CypherQueryAsync([Query] string graphId, CypherQueryRequest request); #region Node [Get("/graph/{graphId}/node/{nodeId}")] Task<Node> GetNodeAsync(string graphId, string nodeId); [Post("/graph/{graphId}/node")] Task<Node> CreateNodeAsync(string graphId, [Body] NodeCreationModel node); [Put("/graph/{graphId}/node/{nodeId}")] Task<Node> UpdateNodeAsync(string graphId, string nodeId, [Body] NodeUpdateModel node); [Put("/graph/{graphId}/node/{nodeId}/merge")] Task<Node> MergeNodeAsync(string graphId, string nodeId, [Body] NodeUpdateModel node); [Delete("/graph/{graphId}/node/{nodeId}")] Task<NodeDeleteResponse?> DeleteNodeAsync(string graphId, string nodeId); ... (clipped 15 lines) 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: No non-success handling: The new HTTP auth handler forwards responses without checking or enriching handling for non-success HTTP statuses, so failures may propagate without actionable context unless handled elsewhere. Referred Code protected override async Task<HttpResponseMessage> SendAsync(HttpRequestMessage requestMessage, CancellationToken cancellationToken) { requestMessage.Headers.TryAddWithoutValidation("Authorization", $"Bearer {_settings.ApiKey}"); var response = await base.SendAsync(requestMessage, cancellationToken).ConfigureAwait(false); #if DEBUG var rawResponse = await response.Content.ReadAsStringAsync(); _logger.LogDebug(rawResponse); #endif return response; } Learn more about managing compliance generic rules or creating your own custom rules
	Generic: Security-First Input Validation and Data Handling Objective: Ensure all data inputs are validated, sanitized, and handled securely to prevent vulnerabilities Status: No request validation: New request DTOs (e.g., `EdgeCreationModel`, `NodeCreationModel` with `EmbeddingInfo`) introduce externally supplied fields without any visible validation/sanitization constraints, so correctness and security depend on validation elsewhere. Referred Code public class EdgeCreationModel { public string? Id { get; set; } public string SourceNodeId { get; set; } = null!; public string TargetNodeId { get; set; } = null!; public string Type { get; set; } = null!; public bool Directed { get; set; } = true; public float? Weight { get; set; } = 1.0f; public object? Properties { get; set; } } 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 · 2026-01-07T23:30:00Z

PR Code Suggestions ✨

Latest suggestions up to dfdec70

Category	Suggestion	Impact
Incremental ^[*]	Return proper not-found response Modify the `GetConversation` method to return an `ActionResult` instead of `ConversationViewModel?`. When a conversation is not found, return an HTTP 404 `NotFound()` result instead of an empty object to provide clearer API responses. src/Infrastructure/BotSharp.OpenAPI/Controllers/Conversation/ConversationController.cs [145-188] -public async Task<ConversationViewModel?> GetConversation([FromRoute] string conversationId, [FromQuery] bool isLoadStates = false) +public async Task<ActionResult<ConversationViewModel>> GetConversation([FromRoute] string conversationId, [FromQuery] bool isLoadStates = false) { var convService = _services.GetRequiredService<IConversationService>(); var userService = _services.GetRequiredService<IUserService>(); var settings = _services.GetRequiredService<PluginSettings>(); var (isAdmin, user) = await userService.IsAdminUser(_user.Id); var filter = new ConversationFilter { Id = conversationId, UserId = !isAdmin ? user?.Id : null, IsLoadLatestStates = isLoadStates }; var conversations = await convService.GetConversations(filter); var conversation = conversations.Items?.FirstOrDefault(); if (conversation == null) { - return new(); + return NotFound(); } user = !string.IsNullOrEmpty(conversation.UserId) ? await userService.GetUser(conversation.UserId) : null; if (user == null) { user = new User { Id = _user.Id, UserName = _user.UserName, FirstName = _user.FirstName, LastName = _user.LastName, Email = _user.Email, Source = "Unknown" }; } var conversationView = ConversationViewModel.FromSession(conversation); conversationView.User = UserViewModel.FromUser(user); conversationView.IsRealtimeEnabled = settings?.Assemblies?.Contains("BotSharp.Core.Realtime") ?? false; return conversationView; } Apply / Chat Suggestion importance[1-10]: 8 __ Why: The suggestion correctly points out that returning an empty object on failure is poor API design. Returning a proper HTTP status code like 404 is a significant improvement for client-side error handling and API semantics, and also has security benefits.	Medium
	Avoid logging raw file data In the `ToString()` method, instead of returning a substring of `FileData`, return a summary string like `""`. This prevents accidentally logging potentially large or sensitive file content. src/Infrastructure/BotSharp.Abstraction/Files/Models/InstructFileModel.cs [32-35] else if (!string.IsNullOrEmpty(FileData)) { - return FileData.SubstringMax(50); + return $"<inline-file-data len={FileData.Length}>"; } Apply / Chat Suggestion importance[1-10]: 7 __ Why: This suggestion correctly identifies a potential security and logging issue where raw file data could be leaked into logs via `ToString()`. Replacing the raw data with a summary is a good practice for security and log management, especially since the PR increased the amount of data being exposed.	Medium
	Prevent null list crashes Revert the `Files` property from a nullable list back to a non-nullable list initialized as empty. This change prevents potential `NullReferenceException` and adheres to best practices for handling collections. src/Infrastructure/BotSharp.OpenAPI/ViewModels/Instructs/Request/InstructMessageModel.cs [13] -public List<InstructFileModel>? Files { get; set; } +public List<InstructFileModel> Files { get; set; } = []; Apply / Chat Suggestion importance[1-10]: 6 __ Why: The suggestion correctly identifies that changing the `Files` property to be nullable is a regression in code quality, as it can lead to `NullReferenceException`. Reverting to a non-nullable, initialized list is a standard best practice that improves robustness.	Low
Possible issue	Make deletes resilient to empty bodies Wrap the return types of `DeleteNodeAsync` and `DeleteEdgeAsync` with `ApiResponse` to prevent potential Refit deserialization failures when the API returns an empty response body (e.g., with a `204 No Content` status). src/Plugins/BotSharp.Plugin.Membase/Services/IMembaseApi.cs [30-45] [Delete("/graph/{graphId}/node/{nodeId}")] -Task<NodeDeleteResponse?> DeleteNodeAsync(string graphId, string nodeId); +Task<ApiResponse<NodeDeleteResponse>> DeleteNodeAsync(string graphId, string nodeId); ... [Delete("/graph/{graphId}/edge/{edgeId}")] -Task<EdgeDeleteResponse> DeleteEdgeAsync(string graphId, string edgeId); +Task<ApiResponse<EdgeDeleteResponse>> DeleteEdgeAsync(string graphId, string edgeId); `[To ensure code accuracy, apply this suggestion manually]` Suggestion importance[1-10]: 8 __ Why: The suggestion correctly identifies a potential runtime deserialization error if the `DELETE` endpoints return a `204 No Content` status, which is a common REST practice. Using `ApiResponse<T>` makes the client more robust by allowing it to handle non-success status codes and empty response bodies gracefully.	Medium
Possible issue	Validate configuration before creating URI Add validation to ensure `settings.Host` is a valid absolute URI before configuring the `HttpClient`'s `BaseAddress`. This prevents application startup crashes from invalid configuration and provides a clear error message. src/Plugins/BotSharp.Plugin.Membase/MembasePlugin.cs [18-24] services.AddTransient<MembaseAuthHandler>(); services.AddRefitClient<IMembaseApi>(new RefitSettings { CollectionFormat = CollectionFormat.Multi }) .AddHttpMessageHandler<MembaseAuthHandler>() - .ConfigureHttpClient(c => c.BaseAddress = new Uri(settings.Host)); + .ConfigureHttpClient(c => + { + if (string.IsNullOrWhiteSpace(settings.Host) \|\| + !Uri.TryCreate(settings.Host, UriKind.Absolute, out var baseUri)) + { + throw new InvalidOperationException("Membase Host must be a valid absolute URI."); + } + c.BaseAddress = baseUri; + }); + Apply / Chat Suggestion importance[1-10]: 7 __ Why: The suggestion correctly points out that an invalid `settings.Host` value can cause the application to crash on startup. Adding validation for the URI makes the application more robust and provides clearer error messages for misconfigurations, improving diagnosability.	Medium
More

Previous suggestions

✅ Suggestions up to commit 6bf71f2

Category	Suggestion	Impact
Possible issue	✅ ~~Annotate request body parameter~~ Suggestion Impact: The CypherQueryAsync method signature was updated to annotate the request parameter with [Body], ensuring it is serialized into the POST request body. code diff: [Post("/cypher/execute")] - Task<CypherQueryResponse> CypherQueryAsync([Query] string graphId, CypherQueryRequest request); + Task<CypherQueryResponse> CypherQueryAsync([Query] string graphId, [Body] CypherQueryRequest request); Add the `[Body]` attribute to the `request` parameter in the `CypherQueryAsync` method to ensure it's sent in the HTTP request body. src/Plugins/BotSharp.Plugin.Membase/Services/IMembaseApi.cs [15] -Task<CypherQueryResponse> CypherQueryAsync([Query] string graphId, CypherQueryRequest request); +Task<CypherQueryResponse> CypherQueryAsync([Query] string graphId, [Body] CypherQueryRequest request); `[Suggestion processed]` Suggestion importance[1-10]: 9 __ Why: The suggestion correctly points out the missing `[Body]` attribute, which is critical for the `POST` request to function correctly by sending the request object in the body.	High
	Avoid consuming the response stream Avoid consuming the response stream when logging. After reading the response content for logging, replace it with a new `StringContent` so it can be read again by the caller. src/Plugins/BotSharp.Plugin.Membase/Services/MembaseAuthHandler.cs [20-31] protected override async Task<HttpResponseMessage> SendAsync(HttpRequestMessage requestMessage, CancellationToken cancellationToken) { requestMessage.Headers.TryAddWithoutValidation("Authorization", $"Bearer {_settings.ApiKey}"); var response = await base.SendAsync(requestMessage, cancellationToken).ConfigureAwait(false); #if DEBUG - var rawResponse = await response.Content.ReadAsStringAsync(); - _logger.LogDebug(rawResponse); + if (response.Content != null) + { + var rawResponse = await response.Content.ReadAsStringAsync(cancellationToken); + _logger.LogDebug(rawResponse); + + // Re-wrap the content to make it readable again by the caller. + response.Content = new StringContent(rawResponse, System.Text.Encoding.UTF8, response.Content.Headers.ContentType?.MediaType ?? "application/json"); + } #endif return response; } Suggestion importance[1-10]: 8 __ Why: This suggestion correctly identifies a critical bug where logging the response body consumes the content stream, which would cause deserialization to fail for the actual caller.	Medium
	Ensure handler uses updated settings To ensure `MembaseAuthHandler` uses the latest settings if they become reloadable, register it using a factory in `AddHttpMessageHandler` to resolve it per request. src/Plugins/BotSharp.Plugin.Membase/MembasePlugin.cs [18-24] +services.AddTransient<MembaseAuthHandler>(); +services.AddRefitClient<IMembaseApi>(new RefitSettings + { + CollectionFormat = CollectionFormat.Multi + }) + .AddHttpMessageHandler<MembaseAuthHandler>() + .ConfigureHttpClient(c => c.BaseAddress = new Uri(settings.Host)); - Suggestion importance[1-10]: 5 __ Why: The suggestion correctly identifies a potential DI lifecycle issue with `DelegatingHandler`, but the problem it describes (stale settings) won't occur with the current singleton registration of `MembaseSettings`.	Low
General	Initialize embedding properties Initialize the `Model` and `Vector` properties in the `EmbeddingInfo` class to default values to prevent potential null reference issues. src/Plugins/BotSharp.Plugin.Membase/Models/Requests/NodeCreationModel.cs [24-28] public class EmbeddingInfo { - public string Model { get; set; } - public float[] Vector { get; set; } + public string Model { get; set; } = string.Empty; + public float[] Vector { get; set; } = Array.Empty<float>(); } Suggestion importance[1-10]: 6 __ Why: The suggestion improves robustness by initializing properties to non-null defaults, preventing potential `NullReferenceException` issues during object creation or deserialization.	Low
General	Name tuple return elements Improve readability by naming the return tuple elements in the `GetCodeExecutionConfig` method signature (e.g., `(bool useLock, bool useProcess,` `int timeoutSeconds)`). src/Infrastructure/BotSharp.Abstraction/Coding/Utils/CodingUtil.cs [11] -public static (bool, bool, int) GetCodeExecutionConfig(CodingSettings settings, int defaultTimeoutSeconds = 3) +public static (bool useLock, bool useProcess, int timeoutSeconds) GetCodeExecutionConfig(CodingSettings settings, int defaultTimeoutSeconds = 3) Suggestion importance[1-10]: 4 __ Why: This is a good suggestion for improving code readability and maintainability by naming the elements of the returned tuple, making the method's signature more self-documenting.	Low
Learned best practice	Validate auth header and cancellation Validate the API key before adding the Authorization header and pass the provided cancellation token when reading the response content. src/Plugins/BotSharp.Plugin.Membase/Services/MembaseAuthHandler.cs [20-31] protected override async Task<HttpResponseMessage> SendAsync(HttpRequestMessage requestMessage, CancellationToken cancellationToken) { - requestMessage.Headers.TryAddWithoutValidation("Authorization", $"Bearer {_settings.ApiKey}"); + if (!string.IsNullOrWhiteSpace(_settings.ApiKey)) + { + requestMessage.Headers.TryAddWithoutValidation("Authorization", $"Bearer {_settings.ApiKey}"); + } + var response = await base.SendAsync(requestMessage, cancellationToken).ConfigureAwait(false); #if DEBUG - var rawResponse = await response.Content.ReadAsStringAsync(); + var rawResponse = await response.Content.ReadAsStringAsync(cancellationToken); _logger.LogDebug(rawResponse); #endif return response; } Suggestion importance[1-10]: 5 __ Why: Relevant best practice - Improve defensive coding with precise null and state checks to prevent incorrect behavior.	Low

…atures/refine-membase

Jicheng Lu added 5 commits January 6, 2026 17:26

init

1902b15

Merge branch 'master' of https://github.com/SciSharp/BotSharp into fe…

74ba1d2

…atures/refine-membase

Merge branch 'master' of https://github.com/SciSharp/BotSharp into fe…

45e9bc2

…atures/refine-membase

add logging

caa1cde

add edge and refine node

6bf71f2

qodo-code-review bot added the Review effort 3/5 label Jan 7, 2026

Jicheng Lu added 4 commits January 9, 2026 11:19

add conversation access

a4f5360

Merge branch 'master' of https://github.com/SciSharp/BotSharp into fe…

552a719

…atures/refine-membase

minor change

3dc6dfe

minor change

4e582c2

iceljc marked this pull request as draft January 10, 2026 18:13

revert conv access

dfdec70

iceljc changed the title ~~Features/refine membase~~ refine repository and coding utility Jan 11, 2026

iceljc changed the title ~~refine repository and coding utility~~ refine repository and coding util Jan 11, 2026

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Uh oh!

refine repository and coding util #1273

refine repository and coding util #1273

Uh oh!

iceljc commented Jan 7, 2026 •

edited by qodo-code-review bot

Loading

Uh oh!

qodo-code-review bot commented Jan 7, 2026 •

edited

Loading

Uh oh!

qodo-code-review bot commented Jan 7, 2026 •

edited

Loading

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant

Uh oh!

refine repository and coding util #1273

Are you sure you want to change the base?

refine repository and coding util #1273

Uh oh!

Conversation

iceljc commented Jan 7, 2026 • edited by qodo-code-review bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

PR Type

Description

Diagram Walkthrough

File Walkthrough

Uh oh!

qodo-code-review bot commented Jan 7, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

PR Compliance Guide 🔍

Uh oh!

qodo-code-review bot commented Jan 7, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

PR Code Suggestions ✨

Previous suggestions

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant

iceljc commented Jan 7, 2026 •

edited by qodo-code-review bot

Loading

qodo-code-review bot commented Jan 7, 2026 •

edited

Loading

qodo-code-review bot commented Jan 7, 2026 •

edited

Loading