change code execution response

[iceljc]

PR Type

Enhancement

---

Description

• Changed ExecutionResult from string to CodeInterpretResponse? object

• Updated code execution response handling across multiple services

• Preserved execution result data by storing response object instead of string conversion

• Refactored variable naming for clarity in instruction service

---

Diagram Walkthrough

flowchart LR
  A["CodeExecutionResponseModel"] -->|"ExecutionResult type change"| B["string to CodeInterpretResponse?"]
  C["RuleEngine"] -->|"Pass response object"| A
  D["InstructService"] -->|"Pass response object"| A
  E["InstructionLogHook"] -->|"Convert to string on demand"| F["Logging"]

Loading

File Walkthrough

Relevant files

Enhancement

CodeExecutionResponseModel.cs Change ExecutionResult to CodeInterpretResponse object      src/Infrastructure/BotSharp.Abstraction/Coding/Models/CodeExecutionResponseModel.cs Added using statement for BotSharp.Abstraction.Coding.Responses namespace Changed ExecutionResult property type from string to CodeInterpretResponse? Made property nullable to support cases where execution result may not be available +3/-1      RuleEngine.cs Pass response object to ExecutionResult                                    src/Infrastructure/BotSharp.Core.Rules/Engines/RuleEngine.cs Updated ExecutionResult assignment to pass response object directly instead of calling ToString() Removed string conversion and null coalescing to empty string +1/-1      InstructService.Execute.cs Refactor response handling and variable naming                      src/Infrastructure/BotSharp.Core/Instructs/Services/InstructService.Execute.cs Renamed variable response to instructResult for clarity throughout the method Updated ExecutionResult assignment to pass codeResponse object directly Added logic to set instructResult.Text from ExecutionResult?.ToString() after code execution Improved variable naming consistency and data flow +14/-9    InstructionLogHook.cs Convert ExecutionResult object to string for logging          src/Infrastructure/BotSharp.Logger/Hooks/InstructionLogHook.cs Updated CompletionText assignment to convert ExecutionResult object to string on-demand Added null coalescing to empty string for safe logging +1/-1

---

[qodo-merge-pro]

PR Compliance Guide 🔍

(Compliance updated until commit f1bb73d)

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: 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: 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: Comprehensive Audit Trails Objective: To create a detailed and reliable record of critical system actions for security analysis and compliance. Status: Audit context: The logging of code execution adds result and context but it is unclear whether this covers all critical actions and consistently includes required identifiers for comprehensive audit trails beyond the shown hook. Referred Code new InstructionLogModel { AgentId = agent?.Id, Provider = response.CodeProcessor, Model = string.Empty, TemplateName = response.CodeScript?.Name, UserMessage = response.Text, SystemInstruction = $"Code script name: {response.CodeScript}, Version: {codeScriptVersion.ToString("o")}", CompletionText = response.ExecutionResult?.ToString() ?? string.Empty, States = response.Arguments?.ToDictionary() ?? [], UserId = user?.Id } }); 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: Null handling: The updated flow returns null or empty results without surfacing error context (e.g., when no processor/script), which may hinder debugging if not handled by callers. Referred Code /// <param name="codeOptions"></param> /// <returns></returns> private async Task<InstructResult?> RunCode( Agent agent, RoleDialogModel message, string templateName, CodeInstructOptions? codeOptions) { InstructResult? instructResult = null; if (agent == null) { return instructResult; } var agentService = _services.GetRequiredService<IAgentService>(); var state = _services.GetRequiredService<IConversationStateService>(); var codingSettings = _services.GetRequiredService<CodingSettings>(); var hooks = _services.GetHooks<IInstructHook>(agent.Id); var codeProvider = codeOptions?.Processor ?? codingSettings.CodeExecution?.Processor; ... (clipped 42 lines) 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: Sensitive logs: The logger records UserMessage and States which may contain sensitive data; without sanitization shown in the diff this could risk sensitive information in logs. Referred Code new InstructionLogModel { AgentId = agent?.Id, Provider = response.CodeProcessor, Model = string.Empty, TemplateName = response.CodeScript?.Name, UserMessage = response.Text, SystemInstruction = $"Code script name: {response.CodeScript}, Version: {codeScriptVersion.ToString("o")}", CompletionText = response.ExecutionResult?.ToString() ?? string.Empty, States = response.Arguments?.ToDictionary() ?? [], UserId = user?.Id } }); 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: Input sanitization: Arguments and UserMessage are passed through to execution and logs without visible validation or sanitization in the new code, which may pose injection or data exposure risks. Referred Code UseProcess = useProcess }, cancellationToken: cts.Token); var codeResponse = new CodeExecutionResponseModel { CodeProcessor = processor.Provider, CodeScript = codeScript, Arguments = arguments.DistinctBy(x => x.Key).ToDictionary(x => x.Key, x => x.Value ?? string.Empty), ExecutionResult = response }; foreach (var hook in hooks) { await hook.AfterCodeExecution(agent, codeResponse); 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

---

Previous compliance checks

Compliance check up to commit 9593021

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: 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: 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: Comprehensive Audit Trails Objective: To create a detailed and reliable record of critical system actions for security analysis and compliance. Status: Audit detail scope: The PR adds/improves logging of code execution results but the diff does not show whether all critical actions across the system are comprehensively logged with user ID, timestamp, action, and outcome. Referred Code new InstructionLogModel { AgentId = agent?.Id, Provider = response.CodeProcessor, Model = string.Empty, TemplateName = response.CodeScript?.Name, UserMessage = response.Text, SystemInstruction = $"Code script name: {response.CodeScript}, Version: {codeScriptVersion.ToString("o")}", CompletionText = response.ExecutionResult?.ToString() ?? string.Empty, States = response.Arguments?.ToDictionary() ?? [], UserId = user?.Id } }); 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: Null handling paths: Early returns on null/empty conditions are added but there is no visible logging or context for some paths beyond DEBUG logs, and conversion of ExecutionResult to string may mask errors if ToString() is not informative. Referred Code InstructResult? instructResult = null; if (agent == null) { return instructResult; } var agentService = _services.GetRequiredService<IAgentService>(); var state = _services.GetRequiredService<IConversationStateService>(); var codingSettings = _services.GetRequiredService<CodingSettings>(); var hooks = _services.GetHooks<IInstructHook>(agent.Id); var codeProvider = codeOptions?.Processor ?? codingSettings.CodeExecution?.Processor; codeProvider = !string.IsNullOrEmpty(codeProvider) ? codeProvider : BuiltInCodeProcessor.PyInterpreter; var codeProcessor = _services.GetServices<ICodeProcessor>() .FirstOrDefault(x => x.Provider.IsEqualTo(codeProvider)); if (codeProcessor == null) { #if DEBUG ... (clipped 109 lines) 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: Log content risk: Logging uses response.ExecutionResult?.ToString() which could include sensitive data depending on CodeInterpretResponse.ToString() implementation, not shown in diff. Referred Code new InstructionLogModel { AgentId = agent?.Id, Provider = response.CodeProcessor, Model = string.Empty, TemplateName = response.CodeScript?.Name, UserMessage = response.Text, SystemInstruction = $"Code script name: {response.CodeScript}, Version: {codeScriptVersion.ToString("o")}", CompletionText = response.ExecutionResult?.ToString() ?? string.Empty, States = response.Arguments?.ToDictionary() ?? [], UserId = user?.Id } }); 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: Unvalidated data flow: The PR passes the raw response object into ExecutionResult and logs it elsewhere without visible validation or sanitization in this diff, which may carry external input. Referred Code var codeResponse = new CodeExecutionResponseModel { CodeProcessor = processor.Provider, CodeScript = codeScript, Arguments = arguments.DistinctBy(x => x.Key).ToDictionary(x => x.Key, x => x.Value ?? string.Empty), ExecutionResult = response }; Learn more about managing compliance generic rules or creating your own custom rules

[qodo-merge-pro]

PR Code Suggestions ✨

Explore these optional code suggestions:

Category	Suggestion	Impact
Possible issue	✅ Remove redundant and incorrect text overwrite Suggestion Impact: The commit removed the block that overwrote instructResult.Text with the execution result and standardized hook calls, preserving the earlier Text assignment from codeResponse.Result. code diff: @@ -288,13 +285,8 @@ // After code execution foreach (var hook in hooks) { - await hook.AfterCompletion(agent, instructResult ?? new()); - await hook.AfterCodeExecution(agent, codeExeResponse); - } - - if (instructResult != null) - { - instructResult.Text = codeExeResponse?.ExecutionResult?.ToString() ?? string.Empty; + await hook.AfterCompletion(agent, instructResult); + await hook.AfterCodeExecution(agent, codeExecution); } Remove the block of code that overwrites the instructResult.Text property. This overwrite is redundant and potentially buggy, as the property is already correctly set from codeResponse.Result earlier in the method. src/Infrastructure/BotSharp.Core/Instructs/Services/InstructService.Execute.cs [295-298] -if (instructResult != null) -{ - instructResult.Text = codeExeResponse?.ExecutionResult?.ToString() ?? string.Empty; -} + [Suggestion processed] Suggestion importance[1-10]: 9 __ Why: The suggestion correctly identifies a logical flaw where instructResult.Text is set to codeResponse.Result and then immediately overwritten by codeResponse.ToString(). This overwrite is redundant at best and a bug at worst if ToString() does not return the same value as the Result property, which is a significant risk. Removing this block simplifies the code and prevents a potential bug.	High
High-level	Simplify convoluted result handling logic In InstructService.Execute.cs, the instructResult.Text property is set twice. This suggestion proposes removing the initial redundant assignment to simplify the logic and avoid potential bugs. Examples: src/Infrastructure/BotSharp.Core/Instructs/Services/InstructService.Execute.cs [271-298] instructResult = new InstructResult { MessageId = message.MessageId, Template = context.CodeScript?.Name, Text = codeResponse.Result }; } var codeExeResponse = new CodeExecutionResponseModel { ... (clipped 18 lines) Solution Walkthrough: Before: async Task<InstructResult?> RunCode(...) { // ... var codeResponse = codeProcessor.Run(...); InstructResult? instructResult = null; if (codeResponse?.Success == true) { instructResult = new InstructResult { // ... Text = codeResponse.Result // First assignment }; } var codeExeResponse = new CodeExecutionResponseModel { ExecutionResult = codeResponse }; // ... if (instructResult != null) { // Second assignment, overwriting the first one instructResult.Text = codeExeResponse?.ExecutionResult?.ToString() ?? string.Empty; } return instructResult; } After: async Task<InstructResult?> RunCode(...) { // ... var codeResponse = codeProcessor.Run(...); InstructResult? instructResult = null; if (codeResponse?.Success == true) { instructResult = new InstructResult { // ... // Text is not set here anymore }; } var codeExeResponse = new CodeExecutionResponseModel { ExecutionResult = codeResponse }; // ... if (instructResult != null) { // Single, clear assignment of the final text value instructResult.Text = codeExeResponse?.ExecutionResult?.ToString() ?? string.Empty; } return instructResult; } Suggestion importance[1-10]: 8 __ Why: This suggestion correctly identifies a significant logic flaw where instructResult.Text is assigned and then immediately overwritten, which is either redundant or a bug, and proposes a simplification that improves code clarity and correctness.	Medium
Update