Skip to content

Conversation

@iceljc
Copy link
Collaborator

@iceljc iceljc commented Dec 20, 2025

PR Type

Enhancement


Description

  • Add fallback to return current value when model parameter not found

  • Refactor reasoning effort level parsing logic for clarity

  • Move state retrieval before null check in ParseReasoning method

  • Clear placeholder API key value in gpt-5 configuration


Diagram Walkthrough

flowchart LR
  A["GetModelParameter"] -->|"settings not found"| B["Return current value"]
  C["ParseReasoning"] -->|"retrieve state"| D["Check effort level"]
  D -->|"if empty"| E["Use settings or parameters"]
  E -->|"parse level"| F["Return reasoning config"]
  G["appsettings.json"] -->|"clear placeholder"| H["Empty API key"]
Loading

File Walkthrough

Relevant files
Enhancement
LlmUtility.cs
Add fallback for missing model parameters                               

src/Infrastructure/BotSharp.Abstraction/MLTasks/Utilities/LlmUtility.cs

  • Add else clause to return current value when settings not found
  • Ensures fallback behavior for model parameter retrieval
+4/-0     
ChatCompletionProvider.cs
Refactor reasoning effort level parsing logic                       

src/Plugins/BotSharp.Plugin.OpenAI/Providers/Chat/ChatCompletionProvider.cs

  • Move state retrieval before null check in ParseReasoning method
  • Refactor effort level resolution logic for better readability
  • Simplify parameter lookup by consolidating into conditional block
  • Add agent parameter documentation in method summary
+14/-15 
Configuration changes
appsettings.json
Clear placeholder API key value                                                   

src/WebStarter/appsettings.json

  • Clear placeholder API key value for gpt-5 model configuration
+1/-1     

@iceljc iceljc merged commit 3ab62ce into SciSharp:master Dec 20, 2025
0 of 3 checks passed
@qodo-code-review
Copy link

PR Compliance Guide 🔍

Below is a summary of compliance checks for this PR:

Security Compliance
🟢
No security concerns identified No security vulnerabilities detected by AI analysis. Human verification advised for critical code.
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: Comprehensive Audit Trails

Objective: To create a detailed and reliable record of critical system actions for security analysis
and compliance.

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: 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: Passed

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: Passed

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: Passed

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: Passed

Learn more about managing compliance generic rules or creating your own custom rules

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

@qodo-code-review
Copy link

PR Code Suggestions ✨

Explore these optional code suggestions:

CategorySuggestion                                                                                                                                    Impact
General
Add temperature fallback

When settings are null, set the temperature from agent.LlmConfig.Temperature as
a fallback, similar to how level is handled.

src/Plugins/BotSharp.Plugin.OpenAI/Providers/Chat/ChatCompletionProvider.cs [601-605]

 if (settings == null)
 {
+    temperature = agent?.LlmConfig?.Temperature;
     reasoningEffortLevel = ParseReasoningEffortLevel(level);
     return (temperature, reasoningEffortLevel);
 }
  • Apply / Chat
Suggestion importance[1-10]: 7

__

Why: This is a good suggestion for improving consistency. Since the level has a fallback to the agent's configuration, it makes sense for the temperature to also have a similar fallback when settings are not provided.

Medium
Refactor fallback logic for clarity

Refactor the logic for determining the level to avoid a potentially redundant
variable assignment. First, attempt to get the level from settings.Parameters,
and only then fall back to settings?.EffortLevel.

src/Plugins/BotSharp.Plugin.OpenAI/Providers/Chat/ChatCompletionProvider.cs [612-621]

 if (string.IsNullOrEmpty(level))
 {
-    level = settings?.EffortLevel;
-    if (settings?.Parameters != null
-        && settings.Parameters.TryGetValue("EffortLevel", out var settingValue)
-        && !string.IsNullOrEmpty(settingValue?.Default))
+    if (settings?.Parameters == null
+        || !settings.Parameters.TryGetValue("EffortLevel", out var settingValue)
+        || string.IsNullOrEmpty(settingValue?.Default))
+    {
+        level = settings?.EffortLevel;
+    }
+    else
     {
         level = settingValue.Default;
     }
 }
  • Apply / Chat
Suggestion importance[1-10]: 6

__

Why: The suggestion correctly identifies a confusing piece of logic where a variable assignment can be immediately overwritten, and proposes a change that clarifies the intended fallback priority, improving code readability and maintainability.

Low
Learned
best practice
Simplify defaults and avoid nulls

Since curVal is always available, return it as the default via early return and
make the return type non-nullable (string) to avoid misleading null outcomes.

src/Infrastructure/BotSharp.Abstraction/MLTasks/Utilities/LlmUtility.cs [7-23]

-public static string? GetModelParameter(IDictionary<string, ModelParamSetting>? settings, string key, string curVal)
+public static string GetModelParameter(IDictionary<string, ModelParamSetting>? settings, string key, string curVal)
 {
-    string? res = null;
-
-    if (settings != null
-        && settings.TryGetValue(key, out var value)
-        && value != null)
+    if (settings == null
+        || !settings.TryGetValue(key, out var value)
+        || value == null)
     {
-        res = VerifyModelParameter(curVal, value.Default, value.Options);
-    }
-    else
-    {
-        res = curVal;
+        return curVal;
     }
 
-    return res;
+    return VerifyModelParameter(curVal, value.Default, value.Options) ?? curVal;
 }

[To ensure code accuracy, apply this suggestion manually]

Suggestion importance[1-10]: 6

__

Why:
Relevant best practice - Avoid unnecessary nullable flows and return early with clear defaults to reduce null-related bugs and improve readability.

Low
  • More

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant