Conversation
…resh loop (#14539) TryGetResourceToolMap always returned false because it compared _selectedAppHostPath against _auxiliaryBackchannelMonitor.SelectedAppHostPath, which is only set by explicit select_apphost calls (usually null). After RefreshResourceToolMapAsync sets _selectedAppHostPath to the connection's actual path, the comparison null != "/path/to/AppHost" always failed, so every tools/list call triggered a full refresh instead of using the cached map. Fix: Add ResolvedAppHostPath property to IAuxiliaryBackchannelMonitor that returns SelectedConnection?.AppHostInfo?.AppHostPath, and compare against that. Rename field to _lastRefreshedAppHostPath for clarity. Fixes #14538 Co-authored-by: Mitch Denny <mitch@mitchdeny.com> Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
…ormat Extract version parsing into a testable TryParseVersionOutput method that handles prefixed version strings like 'GitHub Copilot CLI 0.0.397' by taking the last space-separated token before parsing. Fixes #14174 Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Fixes #12303 - Add ProvisioningFailedException to throw clean error messages from AzureBicepResource instead of re-throwing raw RequestFailedException whose Message property includes verbose HTTP status, content, and headers. - Skip redundant error wrapping for DistributedApplicationException in ExecuteStepAsync, since these exceptions already have user-friendly messages that don't need 'Step ... failed: ' prefix prepended. - Update Verify snapshot for the now-cleaner error format. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
|
🚀 Dogfood this PR with:
curl -fsSL https://raw.githubusercontent.com/dotnet/aspire/main/eng/scripts/get-aspire-cli-pr.sh | bash -s -- 14576Or
iex "& { $(irm https://raw.githubusercontent.com/dotnet/aspire/main/eng/scripts/get-aspire-cli-pr.ps1) } 14576" |
There was a problem hiding this comment.
Pull request overview
This pull request addresses issue #12303 by cleaning up Azure deployment error output. The changes prevent verbose HTTP headers, status codes, and raw JSON from Azure SDK RequestFailedException from appearing in error messages, while ensuring that clean, user-friendly error messages are displayed instead.
Changes:
- Introduced
ProvisioningFailedExceptionto carry clean error messages extracted from Azure SDK exceptions - Modified exception handling in the pipeline to avoid double-wrapping
DistributedApplicationExceptionsubtypes - Added comprehensive test coverage to verify HTTP details are not leaked
- Unrelated changes: MCP resource tool caching bug fix and Copilot CLI version parsing refactoring (should be in separate PRs)
Reviewed changes
Copilot reviewed 11 out of 11 changed files in this pull request and generated 6 comments.
Show a summary per file
| File | Description |
|---|---|
| src/Aspire.Hosting.Azure/Exceptions.cs | Adds new ProvisioningFailedException for clean error messages |
| src/Aspire.Hosting.Azure/AzureBicepResource.cs | Throws ProvisioningFailedException with extracted error message instead of raw exception |
| src/Aspire.Hosting/Pipelines/DistributedApplicationPipeline.cs | Avoids wrapping DistributedApplicationException subtypes to prevent verbose double-wrapped errors |
| tests/Aspire.Hosting.Azure.Tests/AzureDeployerTests.cs | Adds test verifying no HTTP details leak into error output |
| tests/Aspire.Hosting.Azure.Tests/Snapshots/AzureDeployerTests.DeployAsync_WithAzureResourcesAndNoEnvironment_Fails.verified.txt | Updates expected error format (removes redundant "Step failed:" wrapper) |
| src/Aspire.Cli/Mcp/McpResourceToolRefreshService.cs | Unrelated: Fixes MCP caching bug by using correct path property |
| src/Aspire.Cli/Backchannel/IAuxiliaryBackchannelMonitor.cs | Unrelated: Adds ResolvedAppHostPath property for MCP caching fix |
| src/Aspire.Cli/Agents/CopilotCli/CopilotCliRunner.cs | Unrelated: Refactors version parsing logic for testability |
| tests/Aspire.Cli.Tests/Commands/AgentMcpCommandTests.cs | Unrelated: Tests for MCP caching fix |
| tests/Aspire.Cli.Tests/TestServices/TestAppHostAuxiliaryBackchannel.cs | Unrelated: Test infrastructure for MCP caching test |
| tests/Aspire.Cli.Tests/Agents/CopilotCliRunnerTests.cs | Unrelated: Tests for Copilot CLI version parsing |
| private Dictionary<string, ResourceToolEntry> _resourceToolMap = new(StringComparer.Ordinal); | ||
| private bool _invalidated = true; | ||
| private string? _selectedAppHostPath; | ||
| private string? _lastRefreshedAppHostPath; |
There was a problem hiding this comment.
This change appears to be unrelated to the PR's stated goal of "Improve Azure deployment error output". This is fixing a bug in MCP resource tool caching where the cached path was being compared against the wrong property. This should be in a separate PR with its own issue tracking and rationale.
| /// <summary> | ||
| /// Gets the AppHost path of the currently resolved connection, or <c>null</c> if no connection is available. | ||
| /// </summary> | ||
| string? ResolvedAppHostPath => SelectedConnection?.AppHostInfo?.AppHostPath; |
There was a problem hiding this comment.
This change is unrelated to the PR's stated goal of improving Azure deployment error output. The addition of the ResolvedAppHostPath property supports an MCP caching bug fix that should be in a separate PR.
| @@ -88,4 +68,37 @@ internal sealed class CopilotCliRunner(ILogger<CopilotCliRunner> logger) : ICopi | |||
| return null; | |||
| } | |||
| } | |||
|
|
|||
| internal static bool TryParseVersionOutput(string output, out SemVersion? version) | |||
| { | |||
| version = null; | |||
| var versionString = output.Trim(); | |||
|
|
|||
| if (string.IsNullOrEmpty(versionString)) | |||
| { | |||
| return false; | |||
| } | |||
|
|
|||
| // Version output may be on the first line if multi-line | |||
| var lines = versionString.Split(['\n', '\r'], StringSplitOptions.RemoveEmptyEntries); | |||
| if (lines.Length > 0) | |||
| { | |||
| versionString = lines[0].Trim(); | |||
| } | |||
|
|
|||
| // Try to extract the version from known formats like "GitHub Copilot CLI 0.0.397" | |||
| var lastSpaceIndex = versionString.LastIndexOf(' '); | |||
| if (lastSpaceIndex >= 0) | |||
| { | |||
| versionString = versionString[(lastSpaceIndex + 1)..]; | |||
| } | |||
|
|
|||
| // Try to parse the version string (may have a 'v' prefix like "v1.2.3") | |||
| if (versionString.StartsWith('v') || versionString.StartsWith('V')) | |||
| { | |||
| versionString = versionString[1..]; | |||
| } | |||
|
|
|||
| return SemVersion.TryParse(versionString, SemVersionStyles.Any, out version); | |||
| } | |||
There was a problem hiding this comment.
This refactoring of version parsing logic into a testable method is unrelated to the PR's stated goal of improving Azure deployment error output. This should be in a separate PR focused on improving Copilot CLI version detection.
| [Fact] | ||
| public async Task McpServer_ListTools_CachesResourceToolMap_WhenConnectionUnchanged() | ||
| { | ||
| // Arrange - Create a mock backchannel and track how many times GetResourceSnapshotsAsync is called | ||
| var getResourceSnapshotsCallCount = 0; | ||
| var mockBackchannel = new TestAppHostAuxiliaryBackchannel | ||
| { | ||
| Hash = "test-apphost-hash", | ||
| IsInScope = true, | ||
| AppHostInfo = new AppHostInformation | ||
| { | ||
| AppHostPath = Path.Combine(_workspace.WorkspaceRoot.FullName, "TestAppHost", "TestAppHost.csproj"), | ||
| ProcessId = 12345 | ||
| }, | ||
| GetResourceSnapshotsHandler = (ct) => | ||
| { | ||
| Interlocked.Increment(ref getResourceSnapshotsCallCount); | ||
| return Task.FromResult(new List<ResourceSnapshot> | ||
| { | ||
| new ResourceSnapshot | ||
| { | ||
| Name = "db-mcp-xyz", | ||
| DisplayName = "db-mcp", | ||
| ResourceType = "Container", | ||
| State = "Running", | ||
| McpServer = new ResourceSnapshotMcpServer | ||
| { | ||
| EndpointUrl = "http://localhost:8080/mcp", | ||
| Tools = | ||
| [ | ||
| new Tool | ||
| { | ||
| Name = "query_db", | ||
| Description = "Query the database" | ||
| } | ||
| ] | ||
| } | ||
| } | ||
| }); | ||
| } | ||
| }; | ||
|
|
||
| _backchannelMonitor.AddConnection(mockBackchannel.Hash, mockBackchannel.SocketPath, mockBackchannel); | ||
|
|
||
| // Act - Call ListTools twice | ||
| var tools1 = await _mcpClient.ListToolsAsync(cancellationToken: _cts.Token).DefaultTimeout(); | ||
| var tools2 = await _mcpClient.ListToolsAsync(cancellationToken: _cts.Token).DefaultTimeout(); | ||
|
|
||
| // Assert - Both calls return the resource tool | ||
| Assert.Contains(tools1, t => t.Name == "db_mcp_query_db"); | ||
| Assert.Contains(tools2, t => t.Name == "db_mcp_query_db"); | ||
|
|
||
| // The resource tool map should be cached after the first call, | ||
| // so GetResourceSnapshotsAsync should only be called once (during the first refresh). | ||
| // Before the fix, TryGetResourceToolMap always returned false due to | ||
| // SelectedAppHostPath vs SelectedConnection path mismatch, causing every | ||
| // ListTools call to trigger a full refresh. | ||
| Assert.Equal(1, getResourceSnapshotsCallCount); | ||
| } |
There was a problem hiding this comment.
This test appears to be unrelated to the PR's stated goal of improving Azure deployment error output. It's testing the MCP resource tool caching behavior that was changed in the unrelated MCP fixes. This should be in a separate PR along with the corresponding production code changes.
| /// <summary> | ||
| /// Gets or sets the function to call when GetResourceSnapshotsAsync is invoked. | ||
| /// If null, returns the ResourceSnapshots list. | ||
| /// </summary> | ||
| public Func<CancellationToken, Task<List<ResourceSnapshot>>>? GetResourceSnapshotsHandler { get; set; } | ||
|
|
||
| public Task<DashboardUrlsState?> GetDashboardUrlsAsync(CancellationToken cancellationToken = default) | ||
| { | ||
| return Task.FromResult(DashboardUrlsState); | ||
| } | ||
|
|
||
| public Task<List<ResourceSnapshot>> GetResourceSnapshotsAsync(CancellationToken cancellationToken = default) | ||
| { | ||
| if (GetResourceSnapshotsHandler is not null) | ||
| { | ||
| return GetResourceSnapshotsHandler(cancellationToken); | ||
| } | ||
|
|
||
| return Task.FromResult(ResourceSnapshots); | ||
| } |
There was a problem hiding this comment.
These test infrastructure changes support the unrelated MCP caching test and should be in a separate PR along with the corresponding test and production code changes.
|
|
||
| [Theory] | ||
| [InlineData("GitHub Copilot CLI 0.0.397", 0, 0, 397)] | ||
| [InlineData("GitHub Copilot CLI 1.2.3", 1, 2, 3)] | ||
| [InlineData("0.0.397", 0, 0, 397)] | ||
| [InlineData("1.2.3", 1, 2, 3)] | ||
| [InlineData("v1.2.3", 1, 2, 3)] | ||
| [InlineData("V1.2.3", 1, 2, 3)] | ||
| [InlineData("GitHub Copilot CLI 0.0.397\nsome other output", 0, 0, 397)] | ||
| [InlineData(" GitHub Copilot CLI 0.0.397 ", 0, 0, 397)] | ||
| public void TryParseVersionOutput_ValidVersionStrings_ReturnsTrue(string input, int major, int minor, int patch) | ||
| { | ||
| var result = CopilotCliRunner.TryParseVersionOutput(input, out var version); | ||
|
|
||
| Assert.True(result); | ||
| Assert.NotNull(version); | ||
| Assert.Equal(major, version.Major); | ||
| Assert.Equal(minor, version.Minor); | ||
| Assert.Equal(patch, version.Patch); | ||
| } | ||
|
|
||
| [Theory] | ||
| [InlineData("")] | ||
| [InlineData(" ")] | ||
| [InlineData("not a version")] | ||
| public void TryParseVersionOutput_InvalidVersionStrings_ReturnsFalse(string input) | ||
| { | ||
| var result = CopilotCliRunner.TryParseVersionOutput(input, out var version); | ||
|
|
||
| Assert.False(result); | ||
| Assert.Null(version); | ||
| } | ||
| } |
There was a problem hiding this comment.
These tests for the CopilotCliRunner version parsing logic are unrelated to the PR's stated goal of improving Azure deployment error output. They should be in a separate PR along with the corresponding production code refactoring.
🎬 CLI E2E Test RecordingsThe following terminal recordings are available for commit
📹 Recordings uploaded automatically from CI run #22174770563 |
Summary
Fixes #12303 — Azure deployment errors are too verbose, duplicated, and include raw HTTP headers/JSON.
Root Cause
When an Azure deployment fails with a
RequestFailedException:AzureBicepResource.ProvisionAzureBicepResourceAsync()catches it, extracts a clean error viaExtractDetailedErrorMessage(), but then doesthrow;— re-throwing the raw exceptionDistributedApplicationPipeline.ExecuteStepAsync()catches it and wraps:throw new InvalidOperationException("Step '{step.Name}' failed: {ex.Message}", ex)—ex.Messageis the verboseRequestFailedException.Message(includes HTTP status, error code, JSON body, response headers)FailAsync(ex.Message)into the step completion textConsoleActivityLoggersplits and prefixes each line, creating a wall of HTTP header noiseChanges
New
ProvisioningFailedException(src/Aspire.Hosting.Azure/Exceptions.cs): Internal exception extendingDistributedApplicationExceptionthat carries the clean, extracted error message instead of the rawRequestFailedException.Message.Throw clean exception (
src/Aspire.Hosting.Azure/AzureBicepResource.cs): Changedthrow;tothrow new ProvisioningFailedException(errorMessage, ex)so the clean message (already extracted byExtractDetailedErrorMessage) propagates up instead of the verbose SDK message.Skip redundant wrapping (
src/Aspire.Hosting/Pipelines/DistributedApplicationPipeline.cs): Addedcatch (DistributedApplicationException) { throw; }before the generic catch inExecuteStepAsyncso thatDistributedApplicationExceptionsubtypes (which already have clean messages) pass through without being wrapped in"Step '...' failed: {verbose}".Unit test (
tests/Aspire.Hosting.Azure.Tests/AzureDeployerTests.cs):DeployAsync_WithRequestFailedException_DoesNotIncludeVerboseHttpDetails— uses aFailingBicepProvisionerwith a mock Azure response to verify no HTTP headers, status lines, or raw JSON leak into error output.Before / After
Before: Error output includes HTTP status, error code, raw JSON content, and full response headers repeated for each failed resource.
After: Error output shows only the clean extracted error message (e.g.,
LocationNotAvailableForResourceType: The provided location 'asia' is not available...).