Skip to content

Conversation

@yileicn
Copy link
Member

@yileicn yileicn commented Jan 23, 2026

PR Type

Enhancement


Description

  • Convert IAgentHook interface methods to async Task-based pattern

  • Change OnAgentLoading to return nullable string for redirection

  • Update all hook implementations to use async/await pattern

  • Refactor HookEmitter calls to properly await async operations


Diagram Walkthrough

flowchart LR
  IAgentHook["IAgentHook Interface"]
  SyncMethods["Sync Methods<br/>bool/void returns"]
  AsyncMethods["Async Methods<br/>Task returns"]
  Implementations["Hook Implementations<br/>A2AAgentHook, BasicAgentHook, etc."]
  HookEmitter["HookEmitter<br/>Async invocation"]
  
  IAgentHook -->|"Convert to async"| AsyncMethods
  AsyncMethods -->|"Implement"| Implementations
  Implementations -->|"Called by"| HookEmitter
  SyncMethods -.->|"Replaced"| AsyncMethods
Loading

File Walkthrough

Relevant files
Enhancement
10 files
IAgentHook.cs
Convert interface methods to async Task pattern                   
+9/-9     
AgentHookBase.cs
Implement async Task-based hook methods                                   
+14/-13 
AgentService.LoadAgent.cs
Refactor hook invocation to async/await pattern                   
+18/-8   
A2AAgentHook.cs
Update A2A hook to async with proper await calls                 
+8/-8     
BasicAgentHook.cs
Convert BasicAgentHook to async Task pattern                         
+4/-2     
RoutingAgentHook.cs
Update routing hook methods to async Task                               
+6/-6     
MCPToolAgentHook.cs
Convert MCP tool hook to async pattern                                     
+2/-2     
SqlPlannerAgentHook.cs
Update SQL planner hook to async Task pattern                       
+9/-3     
CommonAgentHook.cs
Convert test hook to async Task pattern                                   
+2/-2     
PizzaBotAgentHook.cs
Update PizzaBot hook to async Task pattern                             
+2/-2     

@qodo-code-review
Copy link

qodo-code-review bot commented Jan 23, 2026

PR Compliance Guide 🔍

Below is a summary of compliance checks for this PR:

Security Compliance
SSRF via endpoint

Description: Possible SSRF/network pivot risk: GetCapabilitiesAsync(remoteConfig.Endpoint) performs an
outbound call to an endpoint taken from configuration (_a2aSettings.Agents), which could
be abused to reach internal services or unexpected protocols if an attacker can influence
that endpoint value.
A2AAgentHook.cs [37-53]

Referred Code
public override async Task OnAgentLoaded(Agent agent)
{
    // Check if this is an A2A remote agent
    if (agent.Type != AgentType.A2ARemote)
    {
        await base.OnAgentLoaded(agent);
        return;
    }

    var remoteConfig = _a2aSettings.Agents?.FirstOrDefault(x => x.Id == agent.Id);
    if (remoteConfig != null)
    {
        var agentCard = await _a2aService.GetCapabilitiesAsync(remoteConfig.Endpoint);
        if (agentCard != null)
        {
            agent.Name = agentCard.Name;
            agent.Description = agentCard.Description;
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: Comprehensive Audit Trails

Objective: To create a detailed and reliable record of critical system actions for security analysis
and compliance.

Status: Passed

Learn more about managing compliance generic rules or creating your own custom rules

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: Secure Logging Practices

Objective: To ensure logs are useful for debugging and auditing without exposing sensitive
information like PII, PHI, or cardholder data.

Status: Passed

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:
Missing exception handling: The new awaited external call await
_a2aService.GetCapabilitiesAsync(remoteConfig.Endpoint) has no visible error
handling/logging, so transient failures may propagate and break agent loading without
actionable context.

Referred Code
var agentCard = await _a2aService.GetCapabilitiesAsync(remoteConfig.Endpoint);
if (agentCard != null)

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 external endpoint: The PR introduces an awaited call using remoteConfig.Endpoint without any visible
validation/allowlisting, so the endpoint source/trust boundary should be verified to
prevent unsafe outbound calls (e.g., SSRF) depending on configuration controls.

Referred Code
var remoteConfig = _a2aSettings.Agents?.FirstOrDefault(x => x.Id == agent.Id);
if (remoteConfig != null)
{
    var agentCard = await _a2aService.GetCapabilitiesAsync(remoteConfig.Endpoint);
    if (agentCard != null)

Learn more about managing compliance generic rules or creating your own custom rules

  • 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 Jan 23, 2026

PR Code Suggestions ✨

Explore these optional code suggestions:

CategorySuggestion                                                                                                                                    Impact
General
Chain to base async method

Modify OnInstructionLoaded to call the base implementation instead of returning
true directly, ensuring base class logic is preserved.

src/Plugins/BotSharp.Plugin.Planner/SqlGeneration/Hooks/SqlPlannerAgentHook.cs [34]

-return true;
+return await base.OnInstructionLoaded(template, dict);

[To ensure code accuracy, apply this suggestion manually]

Suggestion importance[1-10]: 6

__

Why: The suggestion correctly points out that the overridden method should call the base implementation to ensure base logic is executed, which improves robustness and maintainability.

Low
Learned
best practice
Remove redundant async/await

Remove async/await here and return the base Task directly to avoid extra
allocations/state machine overhead.

src/Infrastructure/BotSharp.Core.A2A/Hooks/A2AAgentHook.cs [26-35]

-public override async Task<bool> OnAgentLoading(ref string id)
+public override Task<bool> OnAgentLoading(ref string id)
 {
     var agentId = id;
     var remoteConfig = _a2aSettings.Agents?.FirstOrDefault(x => x.Id == agentId);
     if (remoteConfig != null)
     {
-        return true;
+        return Task.FromResult(true);
     }
-    return await base.OnAgentLoading(ref id);
+    return base.OnAgentLoading(ref id);
 }
  • Apply / Chat
Suggestion importance[1-10]: 5

__

Why:
Relevant best practice - Avoid unnecessary async state machines when a method only forwards/returns another Task (remove redundant async/await).

Low
  • Update

@adenchen123
Copy link
Contributor

Reviewed

@JackJiang1234
Copy link
Contributor

reviewed

@yileicn yileicn merged commit a02bbcd into SciSharp:master Jan 23, 2026
4 checks passed
@yileicn
Copy link
Member Author

yileicn commented Jan 23, 2026

/describe /review /improve

@qodo-code-review
Copy link

PR Description updated to latest commit (7c63258)

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.

3 participants