Skip to content

add invoke source #1103

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.

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

Merged
merged 2 commits into from
Jul 24, 2025
Merged

Conversation

iceljc
Copy link
Collaborator

@iceljc iceljc commented Jul 15, 2025

PR Type

Enhancement


Description

  • Add InvokeSource enum to track function invocation origins

  • Update function execution hooks with source parameter

  • Modify routing service methods to accept invoke source

  • Enhance realtime hub with source-aware function execution


Diagram Walkthrough

flowchart LR
  A["InvokeSource enum"] --> B["Hook interfaces"]
  B --> C["Routing service"]
  C --> D["Function execution"]
  D --> E["Source-aware processing"]
Loading

File Walkthrough

Relevant files

Copy link

qodo-merge-pro bot commented Jul 15, 2025

PR Reviewer Guide 🔍

(Review updated until commit e52eac5)

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

Design Pattern

Using static class with const strings instead of enum for InvokeSource may lead to type safety issues and potential runtime errors from typos. Consider using a proper enum type for better compile-time validation.

public static class InvokeSource
{
    /// <summary>
    /// Invoke manually
    /// </summary>
    public const string Manual = "manual";

    /// <summary>
    /// Invoke by LLM directly
    /// </summary>
    public const string Llm = "llm";

    /// <summary>
    /// Invoke by agent routing
    /// </summary>
    public const string Routing = "routing";
}
Logic Change

The condition logic has been inverted - now returns early when source is NOT from LLM, which changes the behavior significantly. This should be carefully validated to ensure it matches the intended functionality.

if (from != InvokeSource.Llm || hub.HubConn == null)
{
    return;
}
Breaking Change

Adding a parameter with default value to virtual methods in base class could cause issues with existing implementations that override these methods without the new parameter. All derived classes need to be updated consistently.

public virtual Task OnFunctionExecuting(RoleDialogModel message, string from = InvokeSource.Manual)
    => Task.CompletedTask;

public virtual Task OnFunctionExecuted(RoleDialogModel message, string from = InvokeSource.Manual)
    => Task.CompletedTask;

Copy link

qodo-merge-pro bot commented Jul 15, 2025

PR Code Suggestions ✨

No code suggestions found for the PR.

@iceljc iceljc requested a review from Oceania2018 July 15, 2025 21:56
@iceljc iceljc marked this pull request as draft July 15, 2025 22:24
@iceljc iceljc marked this pull request as ready for review July 24, 2025 16:58
Copy link

PR Code Suggestions ✨

No code suggestions found for the PR.

@Oceania2018 Oceania2018 merged commit a3f0f57 into SciSharp:master Jul 24, 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.

2 participants