Skip to content

refine side car states #1089

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Merged
merged 1 commit into from
Jul 7, 2025
Merged

refine side car states #1089

merged 1 commit into from
Jul 7, 2025
+69 −5

Conversation

iceljc
Copy link
Collaborator

@iceljc iceljc commented Jul 7, 2025 •
edited by qodo-merge-pro bot
Loading

PR Type

Bug fix, Enhancement

Description

  • Fix sidecar state restoration bug in SetCurrentState method

  • Add state inheritance options for sidecar conversations

  • Introduce SideCarOptions model for configuration control

  • Enhance SendMessage method with optional parameters

Changes diagram

flowchart LR
  A["SideCarOptions"] --> B["SendMessage Method"]
  B --> C["RestoreStates Logic"]
  C --> D["State Inheritance"]
  E["SetCurrentState Fix"] --> C
Loading

Changes walkthrough 📝

Relevant files
Enhancement
IConversationSideCar.cs
Update interface with SideCarOptions parameter                     

src/Infrastructure/BotSharp.Abstraction/SideCar/IConversationSideCar.cs

  • Add SideCarOptions parameter to SendMessage method
  • Import SideCar.Models namespace
  • Update method signature with optional parameters
    		• +6/-1     
    SideCarOptions.cs
    Add SideCarOptions configuration model                                     

    src/Infrastructure/BotSharp.Abstraction/SideCar/Models/SideCarOptions.cs

  • Create new SideCarOptions class
  • Add IsInheritStates and InheritStateKeys properties
  • Include static Empty() factory method
    		• +12/-0   
    BotSharpConversationSideCar.cs
    Implement state inheritance and restoration logic               

    src/Infrastructure/BotSharp.Core.SideCar/Services/BotSharpConversationSideCar.cs

  • Add _sideCarOptions field for configuration storage
  • Implement RestoreStates method with inheritance logic
  • Update SendMessage to accept and store options
  • Replace direct state restoration with new logic
    		• +48/-2   
    Configuration changes
    Using.cs
    Add SideCar models namespace import                                           

    src/Infrastructure/BotSharp.Core.SideCar/Using.cs

    • Add global using for BotSharp.Abstraction.SideCar.Models
    		 +2/-1     
    Bug fix
    ConversationStateService.cs
    Fix SetCurrentState parameter bug                                               

    src/Infrastructure/BotSharp.Core/Conversations/Services/ConversationStateService.cs

  • Fix bug in SetCurrentState method parameter usage
  • Change from _curStates.Values to state.Values
    		• +1/-1     
    Need help?
  • Type /help how to ... in the comments thread for any questions about Qodo Merge usage.
  • Check out the documentation for more information.
    •

    @qodo-merge-pro Qodo Merge Pro
    Copy link

    qodo-merge-pro bot commented Jul 7, 2025

    PR Reviewer Guide 🔍

    Here are some key observations to aid the review process:

    ⏱️ Estimated effort to review: 3 🔵🔵🔵⚪⚪
    🧪 No relevant tests
    🔒 No security concerns identified
    ⚡ Recommended focus areas for review

    Logic Bug

    The RestoreStates method modifies the innerStates object directly, which is assigned from prevStates parameter. This could lead to unintended side effects as the original state object is being mutated instead of creating a proper copy.

     
    private void RestoreStates(ConversationState prevStates)
{
    var innerStates = prevStates;
    var state = _services.GetRequiredService<IConversationStateService>();

    if (_sideCarOptions?.IsInheritStates == true)
    {
        var curStates = state.GetCurrentState();
        foreach (var pair in curStates)
        {
            var endNode = pair.Value.Values.LastOrDefault();
            if (endNode == null) continue;

            if (_sideCarOptions?.InheritStateKeys?.Any() == true
                && !_sideCarOptions.InheritStateKeys.Contains(pair.Key))
            {
                continue;
            }

            if (innerStates.ContainsKey(pair.Key))
            {
                innerStates[pair.Key].Values.Add(endNode);
            }
            else
            {
                innerStates[pair.Key] = new StateKeyValue
                {
                    Key = pair.Key,
                    Versioning = pair.Value.Versioning,
                    Readonly = pair.Value.Readonly,
                    Values = [endNode]
                };
            }
        }
    }

    state.SetCurrentState(innerStates);
}
    Thread Safety

    The _sideCarOptions field is stored as an instance variable but could be accessed concurrently by multiple threads. This could lead to race conditions where options from one request affect another concurrent request.

     
    private SideCarOptions? _sideCarOptions;

private bool _enabled = false;
private string _conversationId = string.Empty;

public string Provider => "botsharp";

public BotSharpConversationSideCar(
    IServiceProvider services,
    ILogger<BotSharpConversationSideCar> logger)
{
    _services = services;
    _logger = logger;
}

public bool IsEnabled()
{
    return _enabled;
}

public void AppendConversationDialogs(string conversationId, List<DialogElement> messages)
{
    if (!IsValid(conversationId))
    {
        return;
    }

    var top = _contextStack.Peek();
    top.Dialogs.AddRange(messages);
}

public List<DialogElement> GetConversationDialogs(string conversationId)
{
    if (!IsValid(conversationId))
    {
        return new List<DialogElement>();
    }

    return _contextStack.Peek().Dialogs;
}

public void UpdateConversationBreakpoint(string conversationId, ConversationBreakpoint breakpoint)
{
    if (!IsValid(conversationId))
    {
        return;
    }

    var top = _contextStack.Peek().Breakpoints;
    top.Add(breakpoint);
}

public ConversationBreakpoint? GetConversationBreakpoint(string conversationId)
{
    if (!IsValid(conversationId))
    {
        return null;
    }

    var top = _contextStack.Peek().Breakpoints;
    return top.LastOrDefault();
}

public void UpdateConversationStates(string conversationId, List<StateKeyValue> states)
{
    if (!IsValid(conversationId))
    {
        return;
    }

    var top = _contextStack.Peek();
    top.State = new ConversationState(states);
}

public async Task<RoleDialogModel> SendMessage(string agentId, string text,
    PostbackMessageModel? postback = null,
    List<MessageState>? states = null,
    List<DialogElement>? dialogs = null,
    SideCarOptions? options = null)
{
    _sideCarOptions = options;
    Null Reference

    In the RestoreStates method, there's potential for null reference exceptions when accessing pair.Value.Values.LastOrDefault() and subsequent operations on endNode without proper null checks on the collection itself.

     
    foreach (var pair in curStates)
{
    var endNode = pair.Value.Values.LastOrDefault();
    if (endNode == null) continue;

    if (_sideCarOptions?.InheritStateKeys?.Any() == true
        && !_sideCarOptions.InheritStateKeys.Contains(pair.Key))
    {
        continue;
    }

    if (innerStates.ContainsKey(pair.Key))
    {
        innerStates[pair.Key].Values.Add(endNode);
    }
    else
    {
        innerStates[pair.Key] = new StateKeyValue
        {
            Key = pair.Key,
            Versioning = pair.Value.Versioning,
            Readonly = pair.Value.Readonly,
            Values = [endNode]
        };
    }
}

    @qodo-merge-pro Qodo Merge Pro
    Copy link

    qodo-merge-pro bot commented Jul 7, 2025

    PR Code Suggestions ✨

    Explore these optional code suggestions:

    CategorySuggestion                                                                                                                                    Impact
    General
    Reset options after execution

    The _sideCarOptions field is not reset after method execution, which could cause
    options from previous calls to persist. Reset the field in the AfterExecute
    method or use a local variable instead.

    src/Infrastructure/BotSharp.Core.SideCar/Services/BotSharpConversationSideCar.cs [102-114]

     public async Task<RoleDialogModel> SendMessage(string agentId, string text,
     PostbackMessageModel? postback = null,
     List<MessageState>? states = null,
     List<DialogElement>? dialogs = null,
     SideCarOptions? options = null)
 {
     _sideCarOptions = options;
 
     BeforeExecute(dialogs);
     var response = await InnerExecute(agentId, text, postback, states);
     AfterExecute();
+    _sideCarOptions = null;
     return response;
 }
    • Apply / Chat
    Suggestion importance[1-10]: 9

    __

    Why: The suggestion correctly identifies a state leakage bug where _sideCarOptions persists across calls, potentially causing incorrect behavior in subsequent executions. This is a critical fix for correctness.

    		High
    Possible issue
    Avoid mutating original state object

    The method directly modifies prevStates by assigning it to innerStates, which
    can cause unintended side effects. Create a deep copy of prevStates to avoid
    mutating the original state object.

    src/Infrastructure/BotSharp.Core.SideCar/Services/BotSharpConversationSideCar.cs [192-229]

     private void RestoreStates(ConversationState prevStates)
 {
-    var innerStates = prevStates;
+    var innerStates = new ConversationState(prevStates.Values.ToList());
     var state = _services.GetRequiredService<IConversationStateService>();
 
     if (_sideCarOptions?.IsInheritStates == true)
     {
         var curStates = state.GetCurrentState();
         foreach (var pair in curStates)
         {
             var endNode = pair.Value.Values.LastOrDefault();
             if (endNode == null) continue;
 
             if (_sideCarOptions?.InheritStateKeys?.Any() == true
                 && !_sideCarOptions.InheritStateKeys.Contains(pair.Key))
             {
                 continue;
             }
 
             if (innerStates.ContainsKey(pair.Key))
             {
                 innerStates[pair.Key].Values.Add(endNode);
             }
             else
             {
                 innerStates[pair.Key] = new StateKeyValue
                 {
                     Key = pair.Key,
                     Versioning = pair.Value.Versioning,
                     Readonly = pair.Value.Readonly,
                     Values = [endNode]
                 };
             }
         }
     }
 
     state.SetCurrentState(innerStates);
 }
    • Apply / Chat
    Suggestion importance[1-10]: 8

    __

    Why: The suggestion correctly identifies that prevStates is mutated, which can cause unintended side effects, and proposes creating a copy to ensure the method is pure regarding its input parameter.

    		Medium
    • More

    @iceljc iceljc merged commit 5d5abf5 into SciSharp:master Jul 7, 2025
    4 checks passed
    Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
    Reviewers
    No reviews
    Assignees
    No one assigned
    Labels
    Review effort 3/5
    Projects
    None yet
    Milestone
    No milestone
    Development

    Successfully merging this pull request may close these issues.
    1 participant
    @iceljc