Improve Resource Health error handling#2577
Improve Resource Health error handling#2577chidozieononiwu wants to merge 2 commits intomicrosoft:mainfrom
Conversation
There was a problem hiding this comment.
Pull request overview
Note
Copilot was unable to run its full agentic suite in this review.
Adds a dedicated exception type for Azure Resource Health non-success responses and centralizes HTTP response handling to preserve ARM error details (including improved 409 Conflict guidance), with new unit tests validating conflict behavior for availability status and service health event list flows.
Changes:
- Introduced
ResourceHealthRequestFailedExceptionto retain status code, error code/message, and raw response content. - Replaced
EnsureSuccessStatusCode()with a sharedEnsureResourceHealthSuccessAsync(...)that parses ARM error payloads and throws typed exceptions. - Added unit tests covering 409 Conflict scenarios for list operations and command-level error mapping.
Reviewed changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| tools/Azure.Mcp.Tools.ResourceHealth/src/Services/ResourceHealthService.cs | Centralizes failure parsing/throwing and replaces raw EnsureSuccessStatusCode() calls. |
| tools/Azure.Mcp.Tools.ResourceHealth/src/Services/ResourceHealthRequestFailedException.cs | New exception type to carry ARM failure details and build an error message. |
| tools/Azure.Mcp.Tools.ResourceHealth/src/Commands/BaseResourceHealthCommand.cs | Maps the new exception to status codes and conflict-specific messaging. |
| tools/Azure.Mcp.Tools.ResourceHealth/tests/.../ResourceHealthServiceSsrfValidationTests.cs | Adds a unit test asserting 409 parsing and the new exception details surface correctly. |
| tools/Azure.Mcp.Tools.ResourceHealth/tests/.../ServiceHealthEventsListCommandTests.cs | Adds command test for 409 Conflict response handling/messaging. |
| tools/Azure.Mcp.Tools.ResourceHealth/tests/.../AvailabilityStatusGetCommandTests.cs | Adds command test for 409 Conflict response handling/messaging. |
d3bf5cd to
90fffb2
Compare
jongio
left a comment
There was a problem hiding this comment.
The PR's approach is solid - centralizing error handling via EnsureResourceHealthSuccessAsync and adding ResourceHealthRequestFailedException for non-422 failures is clean. The 409 Conflict guidance is useful.
However, CI is failing across all platforms due to a property name mismatch: the exception class declares ErrorMessage (line 19 of the new file) but BaseResourceHealthCommand.GetErrorMessage and the SSRF test both reference ErrorDetails. This needs to be reconciled - either rename the property back to ErrorDetails or update the two call sites.
Add ResourceHealthRequestFailedException to preserve non-success ARM response details from Resource Health calls. Replace raw EnsureSuccessStatusCode handling with parsed Resource Health failures, including clearer 409 Conflict guidance for provider registration or registration-in-progress cases. Add unit coverage for availability status and service health event conflict responses.
90fffb2 to
49c0d19
Compare
What does this PR do?
Add ResourceHealthRequestFailedException to preserve non-success ARM response details from Resource Health calls.
Replace raw EnsureSuccessStatusCode handling with parsed Resource Health failures, including clearer 409 Conflict guidance for provider registration or registration-in-progress cases.
Add unit coverage for availability status and service health event conflict responses.
Pre-merge Checklist
servers/Azure.Mcp.Server/README.mdand/orservers/Fabric.Mcp.Server/README.mddocumentationREADME.mdchanges running the script./eng/scripts/Process-PackageReadMe.ps1. See Package READMEToolDescriptionEvaluatorand obtained a score of0.4or more and a top 3 ranking for all related test promptsconsolidated-tools.jsonbreaking-changelabelservers/Azure.Mcp.Server/docs/azmcp-commands.md./eng/scripts/Update-AzCommandsMetadata.ps1to update tool metadata inazmcp-commands.md(required for CI)servers/Azure.Mcp.Server/docs/e2eTestPrompts.mdcrypto mining, spam, data exfiltration, etc.)/azp run mcp - pullrequest - liveto run Live Test Pipeline