Skip to content

Conversation

iceljc
Copy link
Collaborator

@iceljc iceljc commented Jul 9, 2025

PR Type

Bug fix


Description

  • Detour rate limit for phone channel conversations

  • Move conversation state service initialization earlier

  • Skip message frequency check for phone channel


Changes diagram

flowchart LR
  A["Rate Limit Hook"] --> B["Get Channel State"]
  B --> C{"Channel == Phone?"}
  C -->|Yes| D["Skip Frequency Check"]
  C -->|No| E["Apply Rate Limit"]
Loading

Changes walkthrough 📝

Relevant files
Bug fix
RateLimitConversationHook.cs
Detour phone channel from rate limiting                                   

src/Infrastructure/BotSharp.Logger/Hooks/RateLimitConversationHook.cs

  • Move conversation state service initialization to top of method
  • Add channel check to bypass frequency rate limiting for phone
    conversations
  • Reorganize code structure for better flow
  • +4/-4     

    Need help?
  • Type /help how to ... in the comments thread for any questions about Qodo Merge usage.
  • Check out the documentation for more information.
  • Copy link

    qodo-merge-pro bot commented Jul 9, 2025

    PR Reviewer Guide 🔍

    Here are some key observations to aid the review process:

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

    Null Reference

    The channel state retrieved from states.GetState("channel") could be null, but there's no null check before using it in comparisons. This could cause NullReferenceException when the channel state is not set.

    var channel = states.GetState("channel");
    
    // Check max input length
    var charCount = message.Content.Length;
    if (charCount > rateLimit.MaxInputLengthPerRequest)
    {
        message.Content = $"The number of characters in your message exceeds the system maximum of {rateLimit.MaxInputLengthPerRequest}";
        message.StopCompletion = true;
        return;
    }
    
    if (Dialogs == null)
    {
        return;
    }
    
    // Check message sending frequency
    var userSents = Dialogs.Where(x => x.Role == AgentRole.User)
        .TakeLast(2).ToList();
    
    if (channel != ConversationChannel.Phone && userSents.Count > 1)
    Logic Gap

    The frequency check condition combines channel filtering with user message count check in a single if statement. If channel is Phone, the entire frequency check is skipped even when userSents.Count <= 1, which might not be the intended behavior.

    if (channel != ConversationChannel.Phone && userSents.Count > 1)
    {
        var seconds = (DateTime.UtcNow - userSents.First().CreatedAt).TotalSeconds;
        if (seconds < rateLimit.MinTimeSecondsBetweenMessages)
        {
            message.Content = "Your message sending frequency exceeds the frequency specified by the system. Please try again later.";
            message.StopCompletion = true;
            return;
        }
    }

    Copy link

    qodo-merge-pro bot commented Jul 9, 2025

    PR Code Suggestions ✨

    Explore these optional code suggestions:

    CategorySuggestion                                                                                                                                    Impact
    Possible issue
    Cast channel state to enum type

    The GetState method returns an object that needs to be cast to the appropriate
    type. Without proper casting, the comparison with ConversationChannel.Phone may
    fail or behave unexpectedly.

    src/Infrastructure/BotSharp.Logger/Hooks/RateLimitConversationHook.cs [31]

    -var channel = states.GetState("channel");
    +var channel = (ConversationChannel)states.GetState("channel");
    • Apply / Chat
    Suggestion importance[1-10]: 7

    __

    Why: The suggestion correctly identifies a potential typing issue where comparing an inferred type from GetState with an enum ConversationChannel.Phone can be unreliable; explicitly casting to the enum type improves type safety and code clarity.

    Medium
    • More

    @iceljc iceljc merged commit 1821772 into SciSharp:master Jul 9, 2025
    3 of 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