Fix membase json

[iceljc]

No description provided.

[qodo-code-review]

PR Summary by Qodo

Fix Membase Refit JSON deserialization and improve missing-model error

🐞 Bug fix ✨ Enhancement 🕐 10-20 Minutes

AI Description

• Configure Membase Refit client to preserve JsonElement values in dictionary payloads.
• Avoid silent graph/query parsing failures caused by inferred-type deserialization.
• Throw a clear error when OpenAI model settings are missing and no API key is provided.

Diagram

graph TD
  A(["MembasePlugin DI"]) --> B["Refit IMembaseApi"] --> C["Membase HTTP API"]
  B --> D["System.Text.Json (Web)"]
  E(["Graph/Query consumers"]) --> B
  F(["ProviderHelper"]) --> G["ILlmProviderService"] --> H[("LlmProviders config")]
  F --> I["OpenAIClient"]

  subgraph Legend
    direction LR
    _svc([Service/Component]) ~~~ _api[API Client] ~~~ _db[(Configuration Store)]
  end

Loading

High-Level Assessment

The following are alternative approaches to this PR:

1. Update consumers to handle inferred types

• ➕ No need to customize Refit serialization settings
• ➕ Potentially more flexible if API schema varies
• ➖ Touches multiple parsing sites and increases surface area for bugs
• ➖ Harder to maintain consistent behavior (JsonElement vs primitives vs dictionaries)

2. Custom converter to re-wrap inferred values into JsonElement

• ➕ Keeps Refit defaults while restoring expected shapes
• ➖ More custom code than necessary
• ➖ Converter correctness/edge cases become your responsibility

3. Switch to explicit DTOs instead of Dictionary

• ➕ Strong typing prevents shape drift and runtime casting issues
• ➕ Better compile-time safety and testability
• ➖ Requires broader schema modeling and more code changes
• ➖ May be impractical if responses are highly dynamic

Recommendation: The PR’s approach (pin the Membase Refit client to plain System.Text.Json Web options) is the most targeted and lowest-risk fix because it restores the pre-Refit-8 behavior that existing consumers rely on. Consider moving toward explicit DTOs long-term if the Membase response schemas are stable enough.

Files changed (3) +10 / -2

Enhancement (1) +4 / -0

ProviderHelper.csFail fast when provider/model settings are missing +4/-0

Fail fast when provider/model settings are missing

• Adds a guard that throws an InvalidOperationException with a clear message when no LlmProviders settings exist for the requested provider/model and no API key is supplied. Prevents a later NullReferenceException when constructing the OpenAIClient.

src/Plugins/BotSharp.Plugin.OpenAI/Providers/ProviderHelper.cs

Bug fix (2) +6 / -2

IMembaseApi.csRename CompletePgtExternal request body parameter +1/-1

Rename CompletePgtExternal request body parameter

• Renames the CompletePgtExternalAsync body parameter from 'emptyBody' to 'body'. This improves clarity and avoids implying that the endpoint expects a meaningless payload.

src/Plugins/BotSharp.Plugin.Membase/Interfaces/IMembaseApi.cs

MembasePlugin.csForce Refit client to use plain System.Text.Json Web options +5/-1

Force Refit client to use plain System.Text.Json Web options

• Configures the IMembaseApi Refit client with explicit SystemTextJsonContentSerializer using JsonSerializerDefaults.Web. This avoids Refit 8's inferred-type converter so Dictionary<string, object?> values remain JsonElement for downstream graph/query parsing.

src/Plugins/BotSharp.Plugin.Membase/MembasePlugin.cs

[qodo-code-review]

Code Review by Qodo

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

1. Whitespace API key bypass 🐞 Bug ≡ Correctness

Description

ProviderHelper.GetClient checks string.IsNullOrEmpty(apiKey), so a whitespace-only apiKey bypasses
the new missing-settings guard and is still used to construct ApiKeyCredential. This can override a
valid configured settings.ApiKey and cause late authentication failures instead of the intended
clear configuration error.

Code

src/Plugins/BotSharp.Plugin.OpenAI/Providers/ProviderHelper.cs[R12-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

The new guard only checks IsNullOrEmpty(apiKey) and the credential selection uses `apiKey ??
settings!.ApiKey`, which prefers any non-null value (including whitespace) over the configured key;
this directly enables the bypass/override behavior described.

src/Plugins/BotSharp.Plugin.OpenAI/Providers/ProviderHelper.cs[8-19]

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` currently treats whitespace-only `apiKey` values as present because it uses `string.IsNullOrEmpty(apiKey)`. This allows invalid keys (e.g., `"   "`) to bypass the new guard and also overrides a valid configured `settings.ApiKey`, producing confusing downstream authentication failures.

### Issue Context
This was introduced with the new early validation logic added to `GetClient`.

### Fix Focus Areas
- src/Plugins/BotSharp.Plugin.OpenAI/Providers/ProviderHelper.cs[8-19]

### Suggested fix
- Normalize the candidate key and validate the *effective* key:
 - Use `string.IsNullOrWhiteSpace(apiKey)` (and optionally `apiKey = apiKey?.Trim()`)
 - Select `effectiveApiKey = !string.IsNullOrWhiteSpace(apiKey) ? apiKey.Trim() : settings?.ApiKey`
 - Throw if `string.IsNullOrWhiteSpace(effectiveApiKey)`
 - Construct `ApiKeyCredential(effectiveApiKey)`

ⓘ 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]

1. Whitespace api key bypass 🐞 Bug ≡ Correctness

ProviderHelper.GetClient checks string.IsNullOrEmpty(apiKey), so a whitespace-only apiKey bypasses
the new missing-settings guard and is still used to construct ApiKeyCredential. This can override a
valid configured settings.ApiKey and cause late authentication failures instead of the intended
clear configuration error.

Agent Prompt

### Issue description
`ProviderHelper.GetClient` currently treats whitespace-only `apiKey` values as present because it uses `string.IsNullOrEmpty(apiKey)`. This allows invalid keys (e.g., `"   "`) to bypass the new guard and also overrides a valid configured `settings.ApiKey`, producing confusing downstream authentication failures.

### Issue Context
This was introduced with the new early validation logic added to `GetClient`.

### Fix Focus Areas
- src/Plugins/BotSharp.Plugin.OpenAI/Providers/ProviderHelper.cs[8-19]

### Suggested fix
- Normalize the candidate key and validate the *effective* key:
  - Use `string.IsNullOrWhiteSpace(apiKey)` (and optionally `apiKey = apiKey?.Trim()`)
  - Select `effectiveApiKey = !string.IsNullOrWhiteSpace(apiKey) ? apiKey.Trim() : settings?.ApiKey`
  - Throw if `string.IsNullOrWhiteSpace(effectiveApiKey)`
  - Construct `ApiKeyCredential(effectiveApiKey)`

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