Skip to content

Conversation

@iceljc
Copy link
Collaborator

@iceljc iceljc commented Oct 17, 2025

PR Type

Enhancement


Description

  • Add JSON serialization attributes to code and file instruction options

  • Configure null value handling and custom property names for options classes

  • Pass code and file options through conversation state in instruction completion

  • Remove redundant file options state assignment from execute method


Diagram Walkthrough

flowchart LR
  A["InstructMessageModel"] -->|"CodeOptions & FileOptions"| B["InstructModeController"]
  B -->|"SetState"| C["ConversationStateService"]
  C -->|"State Data"| D["InstructService.Execute"]
  E["CodeInstructOptions"] -->|"JsonIgnore & JsonPropertyName"| F["JSON Serialization"]
  G["FileInstructOptions"] -->|"JsonIgnore & JsonPropertyName"| F
Loading

File Walkthrough

Relevant files
Enhancement
CodeInstructOptions.cs
Add JSON serialization attributes to code options               

src/Infrastructure/BotSharp.Abstraction/Instructs/Options/CodeInstructOptions.cs

  • Add JsonIgnore attribute with WhenWritingNull condition to all
    properties
  • Add JsonPropertyName attributes for snake_case serialization
  • Properties: processor, code_script_name, arguments
+8/-0     
FileInstructOptions.cs
Add JSON serialization attributes to file options               

src/Infrastructure/BotSharp.Abstraction/Instructs/Options/FileInstructOptions.cs

  • Add JsonIgnore attribute with WhenWritingNull condition to Processor
    property
  • Add JsonPropertyName attribute for snake_case serialization
+2/-0     
InstructModeController.cs
Add code and file options to state management                       

src/Infrastructure/BotSharp.OpenAPI/Controllers/InstructModeController.cs

  • Add SetState calls for code_options and file_options from input model
  • Pass instruction options through conversation state with external
    source
  • Enable proper state propagation for code and file instruction options
+3/-1     
Bug fix
InstructService.Execute.cs
Remove redundant file options state assignment                     

src/Infrastructure/BotSharp.Core/Instructs/Services/InstructService.Execute.cs

  • Remove redundant state.SetState("fileOptions",
    fileOptions.ConvertToString()) line
  • Simplifies state management by eliminating duplicate file options
    assignment
+0/-1     

@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 <!-- /create_ticket --create_ticket=true -->

</details></td></tr>
Codebase Duplication Compliance
Codebase context is not defined

Follow the guide to enable codebase context checks.

Custom Compliance
No custom compliance provided

Follow the guide to enable custom compliance check.

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
Possible issue
Explicitly serialize objects before state storage

Explicitly serialize the input.CodeOptions and input.FileOptions objects to a
string using ConvertToString() before passing them to SetState to ensure they
are stored correctly.

src/Infrastructure/BotSharp.OpenAPI/Controllers/InstructModeController.cs [36-37]

-.SetState("code_options", input.CodeOptions, source: StateSource.External)
-.SetState("file_options", input.FileOptions, source: StateSource.External);
+.SetState("code_options", input.CodeOptions?.ConvertToString(), source: StateSource.External)
+.SetState("file_options", input.FileOptions?.ConvertToString(), source: StateSource.External);
  • Apply / Chat
Suggestion importance[1-10]: 8

__

Why: The suggestion correctly identifies a potential bug where complex objects might be improperly stored in the state. The proposed fix to serialize them using ConvertToString() is consistent with a pattern seen in code removed elsewhere in this PR, indicating this was a likely oversight.

Medium
Learned
best practice
Add null-safe state defaults

Guard nullable inputs and default to safe fallbacks before persisting to state
(e.g., empty string or empty options object).

src/Infrastructure/BotSharp.OpenAPI/Controllers/InstructModeController.cs [29-37]

-.SetState("provider", input.Provider, source: StateSource.External)
-            .SetState("model", input.Model, source: StateSource.External)
-            .SetState("model_id", input.ModelId, source: StateSource.External)
-            .SetState("instruction", input.Instruction, source: StateSource.External)
-            .SetState("input_text", input.Text, source: StateSource.External)
-            .SetState("template_name", input.Template, source: StateSource.External)
-            .SetState("channel", input.Channel, source: StateSource.External)
-            .SetState("code_options", input.CodeOptions, source: StateSource.External)
-            .SetState("file_options", input.FileOptions, source: StateSource.External);
+.SetState("provider", input.Provider ?? string.Empty, source: StateSource.External)
+            .SetState("model", input.Model ?? string.Empty, source: StateSource.External)
+            .SetState("model_id", input.ModelId ?? string.Empty, source: StateSource.External)
+            .SetState("instruction", input.Instruction ?? string.Empty, source: StateSource.External)
+            .SetState("input_text", input.Text ?? string.Empty, source: StateSource.External)
+            .SetState("template_name", input.Template ?? string.Empty, source: StateSource.External)
+            .SetState("channel", input.Channel ?? string.Empty, source: StateSource.External)
+            .SetState("code_options", input.CodeOptions ?? new CodeInstructOptions(), source: StateSource.External)
+            .SetState("file_options", input.FileOptions ?? new FileInstructOptions(), source: StateSource.External);

[To ensure code accuracy, apply this suggestion manually]

Suggestion importance[1-10]: 6

__

Why:
Relevant best practice - Ensure nullability guards before storing possibly-null inputs into state to avoid downstream null dereferences.

Low
  • More

@iceljc iceljc merged commit c9a091d into SciSharp:master Oct 17, 2025
4 checks passed
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