Skip to content
Merged
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Prev Previous commit
Update McpInspectorResource to prefer https over http endpoint
Co-authored-by: aaronpowell <[email protected]>
  • Loading branch information
Copilot and aaronpowell committed Nov 23, 2025
commit af15c3ec2f76f4ca6818898bd4330e7f339a0017
Original file line number Diff line number Diff line change
Expand Up @@ -67,7 +67,7 @@ internal void AddMcpServer(IResourceWithEndpoints mcpServer, bool isDefault, Mcp

McpServerMetadata item = new(
mcpServer.Name,
mcpServer.GetEndpoint("http") ?? throw new InvalidOperationException($"The MCP server {mcpServer.Name} must have a 'http' endpoint defined."),
mcpServer.GetEndpoint("https") ?? mcpServer.GetEndpoint("http") ?? throw new InvalidOperationException($"The MCP server {mcpServer.Name} must have an 'https' or 'http' endpoint defined."),
Copy link
Contributor

Choose a reason for hiding this comment

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

This never returns null

Copy link
Member

Choose a reason for hiding this comment

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

What happens if there's no registered HTTPS endpoint though?

transportType,
path);

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -407,4 +407,88 @@ public void CombineHandlesMultipleSegmentsAndDoesNotEncodeSlashes()
var expected = new Uri("http://localhost:1234/route/mcp/nested/path");
Assert.Equal(expected, result);
}

[Fact]
public void WithMcpServerSupportsHttpsEndpoint()
{
// Arrange
var appBuilder = DistributedApplication.CreateBuilder();

// Create a mock MCP server resource with https endpoint (uses name "https")
var mockServer = appBuilder.AddProject<Projects.CommunityToolkit_Aspire_Hosting_McpInspector_McpServer>("mcpServer")
.WithHttpsEndpoint(name: "https");

// Act
var inspector = appBuilder.AddMcpInspector("inspector")
Copy link

Copilot AI Nov 23, 2025

Choose a reason for hiding this comment

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

Call to obsolete method AddMcpInspector.

Suggested change
var inspector = appBuilder.AddMcpInspector("inspector")
var inspector = appBuilder.AddMcpInspectorResource("inspector")

Copilot uses AI. Check for mistakes.
Copy link

Copilot AI Nov 23, 2025

Choose a reason for hiding this comment

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

This assignment to inspector is useless, since its value is never read.

Suggested change
var inspector = appBuilder.AddMcpInspector("inspector")
appBuilder.AddMcpInspector("inspector")

Copilot uses AI. Check for mistakes.
.WithMcpServer(mockServer, isDefault: true);

using var app = appBuilder.Build();

// Assert
var appModel = app.Services.GetRequiredService<DistributedApplicationModel>();

var inspectorResource = Assert.Single(appModel.Resources.OfType<McpInspectorResource>());
Assert.Equal("inspector", inspectorResource.Name);

Assert.Single(inspectorResource.McpServers);
Assert.NotNull(inspectorResource.DefaultMcpServer);
Assert.Equal("mcpServer", inspectorResource.DefaultMcpServer.Name);

// Verify that the endpoint was successfully resolved (https should be preferred)
var serverMetadata = inspectorResource.McpServers.Single();
Assert.NotNull(serverMetadata.Endpoint);
Assert.Equal("https", serverMetadata.Endpoint.EndpointName);
}

[Fact]
public void WithMcpServerPrefersHttpsOverHttp()
{
// Arrange
var appBuilder = DistributedApplication.CreateBuilder();

// Create a mock MCP server resource with both https and http endpoints
// AddProject creates "http" by default, we add "https" with explicit name
var mockServer = appBuilder.AddProject<Projects.CommunityToolkit_Aspire_Hosting_McpInspector_McpServer>("mcpServer")
.WithHttpsEndpoint(name: "https");

// Act
var inspector = appBuilder.AddMcpInspector("inspector")
Copy link

Copilot AI Nov 23, 2025

Choose a reason for hiding this comment

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

Call to obsolete method AddMcpInspector.

Copilot uses AI. Check for mistakes.
Copy link

Copilot AI Nov 23, 2025

Choose a reason for hiding this comment

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

This assignment to inspector is useless, since its value is never read.

Suggested change
var inspector = appBuilder.AddMcpInspector("inspector")
appBuilder.AddMcpInspector("inspector")

Copilot uses AI. Check for mistakes.
.WithMcpServer(mockServer, isDefault: true);

using var app = appBuilder.Build();

// Assert
var appModel = app.Services.GetRequiredService<DistributedApplicationModel>();

var inspectorResource = Assert.Single(appModel.Resources.OfType<McpInspectorResource>());
var serverMetadata = inspectorResource.McpServers.Single();

// Verify the endpoint is the https one (https should be preferred)
Assert.Equal("https", serverMetadata.Endpoint.EndpointName);
}

[Fact]
public void WithMcpServerWithBothEndpointsUsesHttps()
{
// Arrange
var appBuilder = DistributedApplication.CreateBuilder();

// AddProject creates both http and https endpoints by default
Copy link

Copilot AI Nov 23, 2025

Choose a reason for hiding this comment

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

The comment states "AddProject creates both http and https endpoints by default", but this is inconsistent with the comment in the test above (line 450) which states "AddProject creates 'http' by default". This creates confusion about the actual behavior. Please verify the behavior and update comments consistently. Based on the code context, AddProject typically creates an HTTP endpoint by default, and HTTPS needs to be added explicitly with WithHttpsEndpoint().

Suggested change
// AddProject creates both http and https endpoints by default
// AddProject creates "http" endpoint by default

Copilot uses AI. Check for mistakes.
var mockServer = appBuilder.AddProject<Projects.CommunityToolkit_Aspire_Hosting_McpInspector_McpServer>("mcpServer");

// Act
var inspector = appBuilder.AddMcpInspector("inspector")
Copy link

Copilot AI Nov 23, 2025

Choose a reason for hiding this comment

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

Call to obsolete method AddMcpInspector.

Copilot uses AI. Check for mistakes.
Copy link

Copilot AI Nov 23, 2025

Choose a reason for hiding this comment

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

This assignment to inspector is useless, since its value is never read.

Suggested change
var inspector = appBuilder.AddMcpInspector("inspector")
appBuilder.AddMcpInspector("inspector")

Copilot uses AI. Check for mistakes.
.WithMcpServer(mockServer, isDefault: true);

using var app = appBuilder.Build();

// Assert
var appModel = app.Services.GetRequiredService<DistributedApplicationModel>();

var inspectorResource = Assert.Single(appModel.Resources.OfType<McpInspectorResource>());
var serverMetadata = inspectorResource.McpServers.Single();

// Verify the endpoint is the https one (preferred when both exist)
Assert.Equal("https", serverMetadata.Endpoint.EndpointName);
}
}
Loading