Skip to content

Conversation

@yileicn
Copy link
Collaborator

@yileicn yileicn commented Oct 20, 2025

PR Type

Enhancement


Description

  • Add deep clone utility extension method for objects

  • Implement JSON-based serialization for deep cloning

  • Apply deep cloning to LoadAgent method for data isolation

  • Prevent unintended mutations of cached agent data


Diagram Walkthrough

flowchart LR
  A["ObjectExtensions<br/>DeepClone Method"] -- "uses" --> B["JsonSerializer<br/>with IgnoreCycles"]
  C["LoadAgent Method"] -- "applies" --> A
  C -- "returns" --> D["Cloned Agent<br/>Instance"]
Loading

File Walkthrough

Relevant files
Enhancement
ObjectExtensions.cs
Add generic deep clone extension method                                   

src/Infrastructure/BotSharp.Abstraction/Utilities/ObjectExtensions.cs

  • New utility class with generic DeepClone extension method
  • Uses JSON serialization/deserialization for deep cloning
  • Configures JsonSerializerOptions with ReferenceHandler.IgnoreCycles
  • Handles null objects gracefully
+25/-0   
AgentService.LoadAgent.cs
Apply deep cloning to LoadAgent result                                     

src/Infrastructure/BotSharp.Core/Agents/Services/AgentService.LoadAgent.cs

  • Apply DeepClone to agent returned from GetAgent method
  • Ensures LoadAgent returns independent copy of agent data
  • Prevents mutations from affecting cached agent instances
+1/-1     

@qodo-code-review
Copy link

qodo-code-review bot commented Oct 20, 2025

PR Compliance Guide 🔍

Below is a summary of compliance checks for this PR:

Security Compliance
Incomplete object clone

Description: JSON-based deep cloning with ReferenceHandler.IgnoreCycles may omit cyclic references
leading to partial object graphs and logic inconsistencies that could be exploited to
bypass validations or authorization checks if security-critical fields are dropped during
cloning.
ObjectExtensions.cs [12-23]

Referred Code
private static readonly JsonSerializerOptions DefaultOptions = new JsonSerializerOptions
{
    ReferenceHandler = ReferenceHandler.IgnoreCycles
};

public static T DeepClone<T>(this T obj) where T : class
{
    if (obj == null) return null;

    var json = JsonSerializer.Serialize(obj, DefaultOptions);
    return JsonSerializer.Deserialize<T>(json, DefaultOptions);
}
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.

  • Update
Compliance status legend 🟢 - Fully Compliant
🟡 - Partial Compliant
🔴 - Not Compliant
⚪ - Requires Further Human Verification
🏷️ - Compliance label

@qodo-code-review
Copy link

qodo-code-review bot commented Oct 20, 2025

PR Code Suggestions ✨

Explore these optional code suggestions:

CategorySuggestion                                                                                                                                    Impact
High-level
Re-evaluate JSON-based deep cloning for performance

The current JSON-based deep cloning in LoadAgent may cause performance issues.
It is recommended to use a more performant alternative, such as a manual clone
method or a specialized library.

Examples:

src/Infrastructure/BotSharp.Abstraction/Utilities/ObjectExtensions.cs [17-23]
        public static T DeepClone<T>(this T obj) where T : class
        {
            if (obj == null) return null;

            var json = JsonSerializer.Serialize(obj, DefaultOptions);
            return JsonSerializer.Deserialize<T>(json, DefaultOptions);
        }
src/Infrastructure/BotSharp.Core/Agents/Services/AgentService.LoadAgent.cs [17]
        var agent = (await GetAgent(id)).DeepClone();

Solution Walkthrough:

Before:

public static class ObjectExtensions
{
    public static T DeepClone<T>(this T obj) where T : class
    {
        // ...
        var json = JsonSerializer.Serialize(obj, ...);
        return JsonSerializer.Deserialize<T>(json, ...);
    }
}

public partial class AgentService
{
    public async Task<Agent> LoadAgent(string id, ...)
    {
        var agent = (await GetAgent(id)).DeepClone();
        // ...
    }
}

After:

public class Agent 
{
    // ... properties
    public Agent ManualClone()
    {
        var newAgent = new Agent { Id = this.Id, Name = this.Name /*, etc. */ };
        // ... manually copy all relevant properties, including deep copies for reference types
        return newAgent;
    }
}

public partial class AgentService
{
    public async Task<Agent> LoadAgent(string id, ...)
    {
        var agent = (await GetAgent(id)).ManualClone();
        // ...
    }
}
Suggestion importance[1-10]: 8

__

Why: The suggestion correctly identifies a significant performance risk by using JSON serialization for deep cloning in the core LoadAgent method, which is a valid and important architectural concern.

Medium
Possible issue
Use a safer reference handler

In ObjectExtensions.cs, change the ReferenceHandler from IgnoreCycles to
Preserve in the JsonSerializerOptions. This ensures that circular references are
handled correctly during the deep cloning process, preventing potential data
loss.

src/Infrastructure/BotSharp.Abstraction/Utilities/ObjectExtensions.cs [12-15]

 private static readonly JsonSerializerOptions DefaultOptions = new JsonSerializerOptions
 {
-    ReferenceHandler = ReferenceHandler.IgnoreCycles
+    ReferenceHandler = ReferenceHandler.Preserve
 };
  • Apply / Chat
Suggestion importance[1-10]: 7

__

Why: The suggestion correctly points out that using ReferenceHandler.IgnoreCycles can lead to data loss for objects with circular references, which contradicts the purpose of a "deep clone". Using ReferenceHandler.Preserve makes the new utility more robust and correct.

Medium
General
Allow configurable JSON serialization options

Refactor the DeepClone method to accept an optional JsonSerializerOptions
parameter. This will allow it to use the application's centrally configured
serialization options from BotSharpOptions, ensuring consistent behavior.

src/Infrastructure/BotSharp.Abstraction/Utilities/ObjectExtensions.cs [12-23]

-private static readonly JsonSerializerOptions DefaultOptions = new JsonSerializerOptions
-{
-    ReferenceHandler = ReferenceHandler.IgnoreCycles
-};
-
-public static T DeepClone<T>(this T obj) where T : class
+public static T DeepClone<T>(this T obj, JsonSerializerOptions options = null) where T : class
 {
     if (obj == null) return null;
 
-    var json = JsonSerializer.Serialize(obj, DefaultOptions);
-    return JsonSerializer.Deserialize<T>(json, DefaultOptions);
+    if (options == null)
+    {
+        options = new JsonSerializerOptions
+        {
+            ReferenceHandler = ReferenceHandler.Preserve
+        };
+    }
+
+    var json = JsonSerializer.Serialize(obj, options);
+    return JsonSerializer.Deserialize<T>(json, options);
 }
  • Apply / Chat
Suggestion importance[1-10]: 7

__

Why: This is a good design suggestion that improves consistency. The DeepClone method should use the application's configurable JsonSerializerOptions rather than its own hardcoded version, ensuring that cloning respects global serialization settings like custom converters.

Medium
Learned
best practice
Safely clone only when non-null

Add a null-conditional when cloning to avoid calling an extension on a null
result from GetAgent.

src/Infrastructure/BotSharp.Core/Agents/Services/AgentService.LoadAgent.cs [17-18]

-var agent = (await GetAgent(id)).DeepClone();
+var fetched = await GetAgent(id);
+var agent = fetched?.DeepClone();
 if (agent == null) return null;
  • Apply / Chat
Suggestion importance[1-10]: 6

__

Why:
Relevant best practice - Ensure nullability guards before property access and avoid potential NullReference by safely cloning results.

Low
  • Update

@JackJiang1234
Copy link
Contributor

reviewed

@yileicn yileicn merged commit 858e188 into SciSharp:master Oct 20, 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