Revert "Fix membase json" by iceljc · Pull Request #1394 · SciSharp/BotSharp

iceljc · 2026-08-04T22:24:12Z

Reverts #1391

qodo-code-review · 2026-08-04T22:25:13Z

PR Summary by Qodo

Revert Membase JSON handling changes and relax OpenAI settings validation

🐞 Bug fix⚙️ Configuration changes🕐 20-40 Minutes

AI Description

• Revert Membase Refit JSON serializer customization and restore prior client configuration.
• Adjust Membase PGT external completion request body parameter naming.
• Remove OpenAI model settings guard to allow API-key-only client creation.

Diagram

graph TD
A["BotSharp app"] --> B["MembasePlugin DI"] --> C["IMembaseApi (Refit)"] --> D{{"Membase API"}}
A --> E["ProviderHelper"] --> F{{"OpenAI API"}}

High-Level Assessment

The following are alternative approaches to this PR:

1. Fix-forward: keep custom Refit serializer but scope it narrowly

➕ Retains intended JsonElement/dictionary behavior for downstream consumers
➕ Avoids global behavior changes if only certain endpoints need special handling
➖ Requires deeper investigation into which payloads break and why
➖ May add per-endpoint complexity (custom content serializer/handlers)

2. Use a typed empty request DTO for PGT completion

➕ Avoids ambiguous object serialization for empty bodies
➕ Improves API contract clarity and reduces runtime surprises
➖ Adds an extra type and may require coordination with server expectations

3. Keep explicit OpenAI settings validation but only when apiKey is absent

➕ Preserves the improved error message when neither settings nor apiKey is available
➕ Avoids potential NullReference exceptions when settings are missing
➖ Slightly more code than the pure revert

Recommendation: If the revert is needed to quickly restore a known-good state, merging is reasonable. However, consider re-introducing the OpenAI guard with conditional logic (only throw when apiKey is not provided) to prevent null-forgiving crashes, and follow up with a typed empty request DTO for the Membase completion endpoint to make serialization behavior explicit.

Files changed (3) +2 / -10

Bug fix (1) +1 / -1

IMembaseApi.csRename PGT external completion body parameter+1/-1

Rename PGT external completion body parameter
• Renames the 'CompletePgtExternalAsync' body parameter from 'body' to 'emptyBody'. This is a non-functional signature change intended to reflect an empty request payload more clearly.
src/Plugins/BotSharp.Plugin.Membase/Interfaces/IMembaseApi.cs

Refactor (1) +0 / -4

ProviderHelper.csRevert explicit settings/apiKey validation when creating OpenAI client+0/-4

Revert explicit settings/apiKey validation when creating OpenAI client
• Removes the guard that threw a descriptive exception when both model settings and an API key were absent. Client creation now proceeds relying on the provided API key or the existing null-forgiving access to 'settings.ApiKey'.
src/Plugins/BotSharp.Plugin.OpenAI/Providers/ProviderHelper.cs

Other (1) +1 / -5

MembasePlugin.csRevert Refit client serializer customization for Membase+1/-5

Revert Refit client serializer customization for Membase
• Removes the custom 'SystemTextJsonContentSerializer' (Web defaults) configuration and restores the prior 'RefitSettings' initialization. Leaves 'CollectionFormat = Multi' as the explicit Refit setting.
src/Plugins/BotSharp.Plugin.Membase/MembasePlugin.cs

qodo-code-review · 2026-08-04T22:28:13Z

Code Review by Qodo

🐞 Bugs (2)📘 Rule violations (0)📜 Skill insights (0)

1. Null settings dereference 🐞 Bug☼ Reliability

Description

ProviderHelper.GetClient can now throw NullReferenceException when
ILlmProviderService.GetSetting returns null and apiKey is null, because it still evaluates
settings!.ApiKey. This regresses from a clear InvalidOperationException explaining how to
configure the model/key.

Code

src/Plugins/BotSharp.Plugin.OpenAI/Providers/ProviderHelper.cs[L12-15]

- if (settings == null && string.IsNullOrEmpty(apiKey))- {- throw new InvalidOperationException($"No LLM model settings found for '{provider}.{model}'. Register the model under LlmProviders (appsettings/user secrets) or pass an api key.");- }

Evidence
LlmProviderService.GetSetting returns null when it can't find provider/model settings. OpenAI
providers frequently pass apiKey: null, and ProviderHelper.GetClient then dereferences
settings!.ApiKey, which will crash when settings are missing.
src/Infrastructure/BotSharp.Core/Infrastructures/LlmProviderService.cs[77-95]
src/Plugins/BotSharp.Plugin.OpenAI/Providers/ProviderHelper.cs[8-15]
src/Plugins/BotSharp.Plugin.OpenAI/Providers/Image/ImageCompletionProvider.Compose.cs[24-29]
src/Plugins/BotSharp.Plugin.OpenAI/Providers/Audio/AudioSynthesisProvider.cs[23-27]

Agent prompt

The issue below was found during a code review. Follow the provided context and guidance below and implement a solution
### Issue description
`ProviderHelper.GetClient` no longer validates the case where both the configured model setting is missing and the caller did not provide an API key, leading to a `NullReferenceException` from `settings!.ApiKey`.
### Issue Context
`ILlmProviderService.GetSetting` explicitly returns null when provider/model settings are not found, and several OpenAI providers call `ProviderHelper.GetClient(..., apiKey: null, ...)`, making the null-settings path reachable.
### Fix Focus Areas
- src/Plugins/BotSharp.Plugin.OpenAI/Providers/ProviderHelper.cs[8-15]
- src/Infrastructure/BotSharp.Core/Infrastructures/LlmProviderService.cs[77-95]
- src/Plugins/BotSharp.Plugin.OpenAI/Providers/Image/ImageCompletionProvider.Compose.cs[24-29]

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

2. Unpinned Membase JSON deserialization 🐞 Bug≡ Correctness

Description

MembasePlugin no longer configures a specific Refit JSON content serializer, but graph
construction relies on Dictionary<string, object?> values being JsonElement for node/edge
objects. If Refit deserializes those object values into inferred CLR types (e.g., nested
dictionaries) instead of JsonElement, GraphBuilder.Build will skip rows and can produce
incomplete/empty graphs.

Code

src/Plugins/BotSharp.Plugin.Membase/MembasePlugin.cs[R29-32]
+ services.AddRefitClient<IMembaseApi>(new RefitSettings
{
CollectionFormat = CollectionFormat.Multi
})

Evidence
GraphBuilder.Build requires JsonElement values and drops rows when it can't read them;
ObjectExtensions.TryGetValue<T> only succeeds when the stored value is exactly T or a
JsonElement. Membase query execution returns Dictionary<string, object?>[] from Refit without
additional normalization, so the Refit deserialization behavior directly determines whether
graph-building can parse the rows.
src/Plugins/BotSharp.Plugin.Membase/MembasePlugin.cs[21-39]
src/Plugins/BotSharp.Plugin.Membase/GraphDb/MembaseGraphDb.cs[41-55]
src/Plugins/BotSharp.Plugin.Membase/Models/Responses/CypherQueryResponse.cs[1-9]
src/Plugins/BotSharp.Plugin.Membase/Services/MembaseInstructionResolver.cs[46-74]
src/Infrastructure/BotSharp.Abstraction/Graph/Utils/GraphBuilder.cs[18-26]
src/Infrastructure/BotSharp.Abstraction/Utilities/ObjectExtensions.cs[76-105]

Agent prompt

The issue below was found during a code review. Follow the provided context and guidance below and implement a solution
### Issue description
The Membase Refit client no longer pins JSON deserialization behavior. Downstream code expects `GraphQueryResult.Values` dictionaries to contain `JsonElement` objects for keys like `sourceNode`, `targetNode`, and `edge`.
### Issue Context
`MembaseGraphDb` passes `CypherQueryResponse.Data` straight through to `GraphQueryResult.Values`. `GraphBuilder.Build` uses `ObjectExtensions.TryGetValue<JsonElement>` and will `continue` (drop the row) when the value isn't a `JsonElement` (or already a `JsonElement`-deserializable `T`).
### Fix Focus Areas
- src/Plugins/BotSharp.Plugin.Membase/MembasePlugin.cs[21-39]
- src/Plugins/BotSharp.Plugin.Membase/GraphDb/MembaseGraphDb.cs[41-55]
- src/Infrastructure/BotSharp.Abstraction/Graph/Utils/GraphBuilder.cs[18-26]
- src/Infrastructure/BotSharp.Abstraction/Utilities/ObjectExtensions.cs[76-105]

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

_{To customize comments, go to the Qodo configuration screen, or learn more in the docs.}

qodo-code-review · 2026-08-04T22:28:17Z

- if (settings == null && string.IsNullOrEmpty(apiKey))
- {
- throw new InvalidOperationException($"No LLM model settings found for '{provider}.{model}'. Register the model under LlmProviders (appsettings/user secrets) or pass an api key.");
- }


1. Null settings dereference 🐞 Bug☼ Reliability
ProviderHelper.GetClient can now throw NullReferenceException when ILlmProviderService.GetSetting returns null and apiKey is null, because it still evaluates settings!.ApiKey. This regresses from a clear InvalidOperationException explaining how to configure the model/key.
Agent Prompt
### Issue description `ProviderHelper.GetClient` no longer validates the case where both the configured model setting is missing and the caller did not provide an API key, leading to a `NullReferenceException` from `settings!.ApiKey`. ### Issue Context `ILlmProviderService.GetSetting` explicitly returns null when provider/model settings are not found, and several OpenAI providers call `ProviderHelper.GetClient(..., apiKey: null, ...)`, making the null-settings path reachable. ### Fix Focus Areas - src/Plugins/BotSharp.Plugin.OpenAI/Providers/ProviderHelper.cs[8-15] - src/Infrastructure/BotSharp.Core/Infrastructures/LlmProviderService.cs[77-95] - src/Plugins/BotSharp.Plugin.OpenAI/Providers/Image/ImageCompletionProvider.Compose.cs[24-29]
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

qodo-code-review · 2026-08-04T22:28:18Z

+ services.AddRefitClient<IMembaseApi>(new RefitSettings
 {
 CollectionFormat = CollectionFormat.Multi
 })


2. Unpinned membase json deserialization 🐞 Bug≡ Correctness
MembasePlugin no longer configures a specific Refit JSON content serializer, but graph construction relies on Dictionary<string, object?> values being JsonElement for node/edge objects. If Refit deserializes those object values into inferred CLR types (e.g., nested dictionaries) instead of JsonElement, GraphBuilder.Build will skip rows and can produce incomplete/empty graphs.
Agent Prompt
### Issue description The Membase Refit client no longer pins JSON deserialization behavior. Downstream code expects `GraphQueryResult.Values` dictionaries to contain `JsonElement` objects for keys like `sourceNode`, `targetNode`, and `edge`. ### Issue Context `MembaseGraphDb` passes `CypherQueryResponse.Data` straight through to `GraphQueryResult.Values`. `GraphBuilder.Build` uses `ObjectExtensions.TryGetValue<JsonElement>` and will `continue` (drop the row) when the value isn't a `JsonElement` (or already a `JsonElement`-deserializable `T`). ### Fix Focus Areas - src/Plugins/BotSharp.Plugin.Membase/MembasePlugin.cs[21-39] - src/Plugins/BotSharp.Plugin.Membase/GraphDb/MembaseGraphDb.cs[41-55] - src/Infrastructure/BotSharp.Abstraction/Graph/Utils/GraphBuilder.cs[18-26] - src/Infrastructure/BotSharp.Abstraction/Utilities/ObjectExtensions.cs[76-105]
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

Revert "Fix membase json"
0c1529a

iceljc merged commit 0e4d262 into masterAug 4, 2026
3 of 5 checks passed

qodo-code-reviewBot reviewed Aug 4, 2026
View reviewed changes

Navigation Menu

Uh oh!

Uh oh!

Revert "Fix membase json" - #1394

Revert "Fix membase json"#1394
iceljc merged 1 commit into
masterfrom
revert-1391-Development

iceljc commented Aug 4, 2026

Uh oh!

Uh oh!

qodo-code-reviewBot commented Aug 4, 2026

Uh oh!

qodo-code-reviewBot commented Aug 4, 2026

Uh oh!

qodo-code-reviewBotAug 4, 2026

Uh oh!

qodo-code-reviewBotAug 4, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant

Uh oh!

Uh oh!

Conversation

iceljc commented Aug 4, 2026

Uh oh!

Uh oh!

qodo-code-reviewBot commented Aug 4, 2026

PR Summary by Qodo

Uh oh!

qodo-code-reviewBot commented Aug 4, 2026

Code Review by Qodo

Uh oh!

qodo-code-reviewBotAug 4, 2026

Choose a reason for hiding this comment

Uh oh!

qodo-code-reviewBotAug 4, 2026

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant