Skip to content

Conversation

@flaviocdc
Copy link
Member

My initial attempt to solve this problem resulted in a lot of repeated code #130, so I decided to try it again, but now extracting the enumeration logic to a central place, and make all frameworks (Foundry, SK, AF) will take a dependency on it.
This way, any fixes made to the enumeration logic, benefits all consumers.

On top of centralizing the logic, I'm also adding parallelism to the tool enumeration process.
If your agent has access to all tools available in A365, then the enumeration process can be very long (I'm seeing it take more than a minute sometimes), doing it in parallel will help making this faster.

flaviocdc and others added 4 commits December 29, 2025 14:06
Add comprehensive test coverage for the shared MCP tool enumeration service:
- Tests for GetAuthTokenAsync token handling
- Tests for EnumerateToolsFromServersAsync including parallel execution
- Tests for EnumerateAllToolsAsync flat list aggregation
- Tests for error handling and invalid server filtering

🤖 Generated with [Claude Code](https://claude.com/claude-code)

Co-Authored-By: Claude Opus 4.5 <[email protected]>
Comprehensive unit tests for McpToolRegistrationService have been added to the AgentFramework, AzureAIFoundry, and SemanticKernel extension test projects. Tests cover argument validation, service orchestration, and tool enumeration scenarios, using Moq for dependency mocking and FluentAssertions for assertions. Moq is now a dependency in all test projects. Service methods in McpToolEnumerationService are marked virtual to enable mocking. These changes improve testability and reliability of the service implementations.
@flaviocdc flaviocdc requested a review from a team as a code owner January 9, 2026 17:25
Copilot AI review requested due to automatic review settings January 9, 2026 17:25
Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

This pull request centralizes MCP (Model Context Protocol) server tool enumeration logic into a new shared service and adds parallelism to improve performance when enumerating tools from multiple servers.

Key Changes:

  • Introduces a new McpToolEnumerationService in the Core library that encapsulates tool enumeration logic with parallel execution support
  • Refactors the three framework-specific McpToolRegistrationService implementations (SemanticKernel, AzureAIFoundry, AgentFramework) to use the centralized service
  • Adds comprehensive unit test coverage for the new service and all refactored consumers

Reviewed changes

Copilot reviewed 12 out of 12 changed files in this pull request and generated 9 comments.

Show a summary per file
File Description
src/Tooling/Core/Services/McpToolEnumerationService.cs New centralized service that handles MCP tool enumeration with parallel execution across multiple servers
src/Tooling/Extensions/SemanticKernel/Services/McpToolRegistrationService.cs Refactored to use the centralized enumeration service instead of duplicated logic
src/Tooling/Extensions/AzureAIFoundry/Services/McpToolRegistrationService.cs Refactored to use the centralized enumeration service; now uses a dictionary lookup for improved performance
src/Tooling/Extensions/AgentFramework/Services/McpToolRegistrationService.cs Refactored to use the centralized enumeration service for both tool server addition and tool retrieval
src/Tests/Microsoft.Agents.A365.Tooling.Core.Tests/Microsoft.Agents.A365.Tooling.Core.Tests.csproj New test project for Core tooling library
src/Tests/Microsoft.Agents.A365.Tooling.Core.Tests/McpToolEnumerationServiceTests.cs Comprehensive unit tests for the new enumeration service including parallelism validation
src/Tests/Microsoft.Agents.A365.Tooling.Extensions.SemanticKernel.Tests/Microsoft.Agents.A365.Tooling.Extensions.SemanticKernel.Tests.csproj Added Moq package reference for test mocking
src/Tests/Microsoft.Agents.A365.Tooling.Extensions.SemanticKernel.Tests/McpToolRegistrationServiceTests.cs Unit tests for refactored SemanticKernel service
src/Tests/Microsoft.Agents.A365.Tooling.Extensions.AzureAIFoundry.Tests/Microsoft.Agents.A365.Tooling.Extensions.AzureAIFoundry.Tests.csproj Added Moq package reference for test mocking
src/Tests/Microsoft.Agents.A365.Tooling.Extensions.AzureAIFoundry.Tests/McpToolRegistrationServiceTests.cs Unit tests for refactored AzureAIFoundry service
src/Tests/Microsoft.Agents.A365.Tooling.Extensions.AgentFramework.Tests/Microsoft.Agents.A365.Tooling.Extensions.AgentFramework.Tests.csproj Added Moq package reference for test mocking
src/Tests/Microsoft.Agents.A365.Tooling.Extensions.AgentFramework.Tests/McpToolRegistrationServiceTests.cs Unit tests for refactored AgentFramework service

Adds IMcpToolEnumerationService interface and updates McpToolEnumerationService to implement it. Refactors McpToolRegistrationService to depend on the interface for improved testability and modularity. Updates service registration to use the new interface and adds AddMcpServices DI extension methods for AgentFramework and AzureFoundry. This enhances extensibility and consistency across frameworks.
Refactored McpToolRegistrationService to eliminate unused dependencies on ILogger and IServiceProvider. Updated constructor to only require IMcpToolEnumerationService and removed related using directives. This simplifies the class and reduces unnecessary coupling.
Copilot AI review requested due to automatic review settings January 9, 2026 19:35
Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 16 out of 16 changed files in this pull request and generated 12 comments.

Refactored McpToolRegistrationService to inject IMcpToolServerConfigurationService via the constructor and store it in a new private field. Updated constructor documentation accordingly. Also removed unused using directives for cleaner code. This prepares the service for enhanced MCP server configuration support.
- Updated McpToolRegistrationService to include additional dependencies for improved functionality.
- Removed obsolete McpToolRegistrationServiceTests class and replaced it with more focused test classes for AddToolServersToAgent and GetMcpToolsAsync methods.
- Enhanced test coverage for McpToolRegistrationService, ensuring proper handling of authentication tokens and tool enumeration.
- Introduced helper methods for setting up mocks and creating instances of services in tests.
- Improved parameter validation in AddToolServersToAgent and GetMcpToolsAsync methods.
Copilot AI review requested due to automatic review settings January 9, 2026 20:22
Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 16 out of 16 changed files in this pull request and generated 14 comments.

flaviocdc and others added 2 commits January 9, 2026 14:38
…ntation

- Replace unused tuple variables with discards (_) in:
  - McpToolEnumerationService.EnumerateAllToolsAsync
  - McpToolRegistrationService.AddToolServersToAgentAsync
  - McpToolEnumerationServiceTests (4 occurrences)
- Standardize copyright headers to MIT License format
- Convert tabs to 4-space indentation for consistency

Co-Authored-By: Claude Opus 4.5 <[email protected]>
- Fix unused 'servers' variable in AgentFramework McpToolRegistrationService
- Fix indentation (tabs to spaces) in AgentFramework McpToolRegistrationService
- Remove unused 'System.Configuration' using directive from SemanticKernel
- Add Microsoft.Agents.A365.Tooling.Core.Tests project to solution file
- Change mock from concrete McpToolEnumerationService to IMcpToolEnumerationService interface in AzureAIFoundry tests
- Remove unused mockAgentClient variable from AzureAIFoundry tests

Co-Authored-By: Claude Opus 4.5 <[email protected]>
/// <param name="turnContext">Turn context for the current request.</param>
/// <param name="providedAuthToken">Optional pre-existing authentication token.</param>
/// <returns>The authentication token.</returns>
public virtual async Task<string> GetAuthTokenAsync(
Copy link
Contributor

@pontemonti pontemonti Jan 9, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

IMO we shouldn't do this, at least not with the current implementation.

  1. If auth token is already set we don't need to call this method. I think the "if null then call GetAgenticUserTokenAsync" pattern makes it easier to see what's going on.
  2. If there is an issue get the authentication token, it's probably better if GetAgenticUserTokenAsync throws an exception about it. The InvalidOperationException we get from here does not contain actionable information by itself.

Can we move the code that calls GetAgenticUserTokenAsync (if authToken is null) into the Enumerate... methods below instead? I think that will work better and also ensures that people calling these APIs from their own code don't have to get an auth token first. #Closed

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed.
I remove this method and I'm simply using AgenticAuthenticationService.GetAgenticUserTokenAsync.

/// <param name="turnContext">Turn context for the current request.</param>
/// <param name="toolOptions">Tool options including user agent configuration.</param>
/// <returns>A tuple containing server configurations and a dictionary mapping server names to their available tools.</returns>
public virtual async Task<(List<MCPServerConfig> Servers, Dictionary<string, IList<McpClientTool>> ToolsByServer)> EnumerateToolsFromServersAsync(
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think authToken and toolOptions should be optional, both in this method and the one below.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

authToken and toolOptions are mandatory in all methods inside IMcpToolServerConfigurationService, so I'm keeping the mandatory now that these 2 methods have been moved to IMcpToolServerConfigurationService.

Does that sound good to you?

Copy link
Contributor

@pontemonti pontemonti left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

See comments

Copilot AI review requested due to automatic review settings January 9, 2026 22:24
Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 17 out of 17 changed files in this pull request and generated 3 comments.

Correct test namespaces from AzureAIFoundry to AzureFoundry. Remove unnecessary using directives. Update test to discard unused toolDefinitions result.
…egrate Tool Enumeration Logic

- Deleted IMcpToolEnumerationService interface and its implementation.
- Refactored McpToolRegistrationService and related services to use IMcpToolServerConfigurationService for tool enumeration.
- Updated unit tests to reflect changes in service dependencies and removed obsolete tests related to token retrieval.
- Enhanced service registration in DI containers by removing references to the deleted enumeration service.
- Improved error handling and logging in tool enumeration methods.
Copilot AI review requested due to automatic review settings January 12, 2026 17:23
Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 16 out of 17 changed files in this pull request and generated 3 comments.

Added Microsoft.Agents.A365.Tooling.Core.Tests project with unit tests for McpToolServerConfigurationService tool enumeration methods. Made key methods virtual for mocking. Updated solution and added required test dependencies.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants