azurebackup: fix telemetry-triaged bugs NEW-1, NEW-3, NEW-4 - #2726
Merged
Shraddha Jain (shrja-ms) merged 5 commits intoMay 27, 2026
Merged
Conversation
Adds a System.CommandLine validator on ProtectableItemListCommand so that an unrecognised --workload-type token is rejected as a 400 ValidationError, with a friendly message naming the supported aliases, instead of leaking through to the service layer where it surfaces as a 500 ArgumentException from the protectable-items REST filter call. Introduces a small WorkloadTypeNormalizer helper whose alias set mirrors exactly the existing service-layer guard in RsvBackupOperations.NormalizeWorkloadTypeForFilter (kept in sync by hand; the helper carries an explicit comment to that effect). The service-layer guard is intentionally left untouched as defense-in-depth. Tests: 19 positive cases (one per canonical token + alias, plus a case-insensitivity case) and 4 negative cases including an OData-injection-style input. The negative tests also assert that the service is never invoked once validation has rejected the input.
Contributor
There was a problem hiding this comment.
Pull request overview
This PR addresses Azure Backup bug NEW-4 by validating --workload-type for azmcp_azurebackup_protectableitem_list at the System.CommandLine boundary, so unrecognized values are rejected as a 400 validation error (instead of surfacing a service-layer exception back to the client).
Changes:
- Added a command-layer validator on
ProtectableItemListCommandto reject unknown--workload-typevalues with a friendly supported-values message. - Introduced
WorkloadTypeNormalizerto centralize the supported workload-type alias set used by the validator. - Added unit tests covering accepted aliases/casing and rejected unknown values; added a server changelog entry.
Invoking Livetests
Copilot submitted PRs are not trustworthy by default. Users with write access to the repo need to validate the contents of this PR before leaving a comment with the text /azp run mcp - pullrequest - live. This will trigger the necessary livetest workflows to complete required validation.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| tools/Azure.Mcp.Tools.AzureBackup/src/Commands/ProtectableItem/ProtectableItemListCommand.cs | Adds --workload-type command validator to fail fast on unknown values. |
| tools/Azure.Mcp.Tools.AzureBackup/src/Services/WorkloadTypeNormalizer.cs | New helper encapsulating workload-type alias recognition and error formatting. |
| tools/Azure.Mcp.Tools.AzureBackup/tests/Azure.Mcp.Tools.AzureBackup.UnitTests/ProtectableItem/ProtectableItemListCommandTests.cs | Adds positive/negative validation tests ensuring service is not called on invalid input. |
| servers/Azure.Mcp.Server/changelog-entries/1779703098177.yaml | Documents the bug fix in the server changelog entry format. |
…bscription name resolution) NEW-1: When both RSV and DPP vault listings fail in ListVaultsAsync and FindUnprotectedResourcesAsync, surface a single meaningful exception instead of leaking AggregateException. If both inners are RequestFailedException with the same HTTP status, throw the inner directly so status/error-code/message reach the user; otherwise wrap both inners in a single InvalidOperationException whose Message includes both. NEW-3: Resolve subscription name -> GUID via ISubscriptionService at the top of every public AzureBackupService method (24 sites) before delegating to RSV/DPP ops that construct ARM ResourceIdentifier. GUID inputs short-circuit via Guid.TryParse so no extra service call happens in the existing happy path. - AzureBackupService ctor gains ISubscriptionService. - 2 NEW-1 unit tests (same-status / different-exceptions) and 2 NEW-3 unit tests (name resolved + GUID short-circuit) added. - AzureBackupSetupTests DI container updated to register ISubscriptionService. - Existing AzureBackupServiceTests migrated to GUID-literal subscriptions so they exercise the Guid.TryParse short-circuit without mock setup.
Shraddha Jain (shrja-ms)
changed the title
- Drop the HasOptionResult gate in the --workload-type validator and read the value via GetValueOrDefault; this ensures whitespace-only inputs (which System.CommandLine may report as 'no result') still fail validation at the command boundary rather than slipping past to the service layer. - IsSupported already returns false for whitespace, so the new validator treats whitespace-only values as 'Unknown workload type' just like other unsupported tokens. Two whitespace InlineData cases added to ExecuteAsync_RejectsUnknownWorkloadType_AsValidationError to lock this in. - Remove the unsubstantiated remark in WorkloadTypeNormalizer that claimed a unit test asserts the two alias sets agree; no such test exists and adding one would require reflection into a private switch in RsvBackupOperations. The 'kept in sync by hand' note remains.
- NEW-1 (alzimmermsft): when both RSV and DPP vault listings fail with the same HTTP status, no longer drop the DPP exception. Both inner messages are now always folded into a single InvalidOperationException so a 403 from RSV and a 403 from DPP (which can carry different ErrorCodes / messages) both reach the user. RSV exception preserved as InnerException. - NEW-3 (alzimmermsft nit): in CreateVaultAsync, run the synchronous VaultTypeResolver.ValidateVaultType check before ResolveSubscriptionIdAsync so invalid --vault-type fails fast without a possible ARM network call. Apply the same reordering to ListProtectableItemsAsync (the IsDpp check). - NEW-4 (alzimmermsft nits): WorkloadTypeNormalizer.SupportedTokensDescription drops the parentheses around aliases, and FormatUnknownMessage now states '(case-insensitive)' in the error message. - Updated ListVaultsAsync_BothFailWithSameStatus_* unit test to assert the new combined-message behavior.
…x-new-bugs-new1-new3-new4
Shraddha Jain (shrja-ms)
enabled auto-merge (squash)
May 26, 2026 17:20
Alan Zimmer (alzimmermsft)
approved these changes
May 27, 2026
1 task
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Fixes three telemetry-triaged Azure Backup MCP bugs from the most recent report (NEW-1, NEW-3, NEW-4) in a single PR.
azmcp_azurebackup_vault_get(andazmcp_azurebackup_governance_find-unprotected) leaked a rawAggregateExceptionto the client, so the actual HTTP status / error code / message from the backends were buried.RequestFailedExceptionis surfaced directly. Otherwise both inner messages are folded into oneInvalidOperationException.--subscriptioncausedFormatExceptionfromAzure.Core.ResourceIdentifier.SubscriptionIdfor everyazmcp_azurebackup_*command. This violated the documented MCP contract that--subscriptionaccepts both subscription IDs and names.AzureBackupServicemethod (24 sites) resolves the input viaISubscriptionService.GetSubscriptionbefore constructing any ARMResourceIdentifier. GUID inputs short-circuit throughGuid.TryParseso the existing happy path makes zero extra calls.azmcp_azurebackup_protectableitem_listwas leaking a 500ArgumentExceptionfrom the service layer when callers passed an unrecognised--workload-type.System.CommandLinevalidator on the command rejects the unknown value as a 400 ValidationError with a friendly message listing the supported aliases. The validator's alias set mirrors exactly the existing service-layer guard (RsvBackupOperations.NormalizeWorkloadTypeForFilter); the service-layer guard is intentionally left in place as defense-in-depth.Tests
Azure.Mcp.Tools.AzureBackup.UnitTests: 584 / 584 passing (was 581 before; +3 net new tests beyond the existing test that was updated to assert the new NEW-1 behavior).ListVaultsAsync_BothFailWithSameStatus_ThrowsInnerRequestFailedException,ListVaultsAsync_BothFailWithDifferentExceptions_ThrowsInvalidOperationWithBothMessages.ListVaultsAsync_WhenSubscriptionIsName_ResolvesToGuidBeforeCallingOps,ListVaultsAsync_WhenSubscriptionIsGuid_DoesNotCallSubscriptionService(assertsISubscriptionService.DidNotReceive().GetSubscription(...)on the GUID happy path).Service.DidNotReceive().AzureBackupServiceTestsmigrated to GUID-literal subscriptions so they exercise theGuid.TryParseshort-circuit without any mock setup.AzureBackupSetupTestsDI container updated to registerISubscriptionServicefor the new constructor parameter.Azure.Mcp.Tools.AzureBackup/src) and server build (servers/Azure.Mcp.Server): clean, 0 warnings / 0 errors.Files
tools/Azure.Mcp.Tools.AzureBackup/src/Services/AzureBackupService.cs— NEW-1 + NEW-3 fixes (newResolveSubscriptionIdAsynchelper, newBuildBothVaultListingsFailedExceptionhelper, ctor now takesISubscriptionService, one-line patches at the top of all 24 public async methods).tools/Azure.Mcp.Tools.AzureBackup/src/Commands/ProtectableItem/ProtectableItemListCommand.cs— NEW-4 validator registration.tools/Azure.Mcp.Tools.AzureBackup/src/Services/WorkloadTypeNormalizer.cs— NEW-4 helper (alias set mirrors RSV service-layer guard).tools/Azure.Mcp.Tools.AzureBackup/tests/Azure.Mcp.Tools.AzureBackup.UnitTests/Services/AzureBackupServiceTests.cs— new NEW-1 + NEW-3 tests, existing tests migrated to GUID subscriptions.tools/Azure.Mcp.Tools.AzureBackup/tests/Azure.Mcp.Tools.AzureBackup.UnitTests/AzureBackupSetupTests.cs— DI container registersISubscriptionService.tools/Azure.Mcp.Tools.AzureBackup/tests/Azure.Mcp.Tools.AzureBackup.UnitTests/ProtectableItem/ProtectableItemListCommandTests.cs— NEW-4 tests.servers/Azure.Mcp.Server/changelog-entries/1779703098177.yaml— changelog entry (3 "Bugs Fixed" bullets).Invoking Livetests
Copilot submitted PRs are not trustworthy by default. Users with
writeaccess to the repo need to validate the contents of this PR before leaving a comment with the text/azp run mcp - pullrequest - live. This will trigger the necessary livetest workflows to complete required validation.