From 69296393607e348de0e4745b6ac56235610669e7 Mon Sep 17 00:00:00 2001 From: Deepali Garg Date: Thu, 16 Jul 2026 15:59:55 -0700 Subject: [PATCH 1/4] Delete server-returned Entra apps on MCP unpublish The platform now returns the Public Clients Entra app registration(s) it cannot delete in the customer tenant (AppIdsToCleanup) from the unpublish response. Change UnpublishServerAsync to return the parsed UnpublishMcpServerResponse instead of bool, and have the develop-mcp unpublish subcommand delete each returned app via Graph (resolving the object id from the app id), mirroring the publish rollback pattern. Degrades gracefully with manual-cleanup guidance when Graph is unavailable or the tenant can't be detected. - New models UnpublishMcpServerResponse + McpServerAppEntry. - New GraphApiService.GetAppObjectIdByAppIdAsync (appId -> application object id). - Regression tests updated for the new return type; added null-response and no-graph-service cleanup coverage. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> --- .../Commands/DevelopMcpCommand.cs | 98 ++++++++++++++++++- .../Models/McpServerAppEntry.cs | 26 +++++ .../Models/UnpublishMcpServerResponse.cs | 40 ++++++++ .../Services/Agent365ToolingService.cs | 30 ++++-- .../Services/GraphApiService.cs | 24 +++++ .../Services/IAgent365ToolingService.cs | 7 +- .../DevelopMcpCommandRegressionTests.cs | 56 ++++++++++- 7 files changed, 267 insertions(+), 14 deletions(-) create mode 100644 src/Microsoft.Agents.A365.DevTools.Cli/Models/McpServerAppEntry.cs create mode 100644 src/Microsoft.Agents.A365.DevTools.Cli/Models/UnpublishMcpServerResponse.cs diff --git a/src/Microsoft.Agents.A365.DevTools.Cli/Commands/DevelopMcpCommand.cs b/src/Microsoft.Agents.A365.DevTools.Cli/Commands/DevelopMcpCommand.cs index 941b1d10..1b45a249 100644 --- a/src/Microsoft.Agents.A365.DevTools.Cli/Commands/DevelopMcpCommand.cs +++ b/src/Microsoft.Agents.A365.DevTools.Cli/Commands/DevelopMcpCommand.cs @@ -1,6 +1,7 @@ // Copyright (c) Microsoft Corporation. // Licensed under the MIT License. +using Microsoft.Agents.A365.DevTools.Cli.Helpers; using Microsoft.Agents.A365.DevTools.Cli.Models; using Microsoft.Agents.A365.DevTools.Cli.Services; using Microsoft.Agents.A365.DevTools.Cli.Services.Evaluate; @@ -37,7 +38,7 @@ public static Command CreateCommand( developMcpCommand.AddCommand(CreateListEnvironmentsSubcommand(logger, toolingService)); developMcpCommand.AddCommand(CreateListServersSubcommand(logger, toolingService)); developMcpCommand.AddCommand(CreatePublishSubcommand(logger, toolingService, graphApiService)); - developMcpCommand.AddCommand(CreateUnpublishSubcommand(logger, toolingService)); + developMcpCommand.AddCommand(CreateUnpublishSubcommand(logger, toolingService, graphApiService)); developMcpCommand.AddCommand(CreateRegisterExternalMcpServerSubcommand(logger, toolingService, graphApiService)); if (evaluationPipelineService is not null) @@ -424,7 +425,8 @@ private static Command CreatePublishSubcommand( /// private static Command CreateUnpublishSubcommand( ILogger logger, - IAgent365ToolingService toolingService) + IAgent365ToolingService toolingService, + GraphApiService? graphApiService) { var command = new Command("unpublish", "Unpublish an MCP server from a Dataverse environment"); @@ -517,9 +519,9 @@ private static Command CreateUnpublishSubcommand( } // Call service - var success = await toolingService.UnpublishServerAsync(envId, serverName); + var response = await toolingService.UnpublishServerAsync(envId, serverName); - if (!success) + if (response is null || !response.IsSuccess) { logger.LogError("Failed to unpublish MCP server {ServerName} from environment {EnvId}", serverName, envId); return; @@ -527,11 +529,99 @@ private static Command CreateUnpublishSubcommand( logger.LogInformation("Successfully unpublished MCP server {ServerName} from environment {EnvId}", serverName, envId); + // The platform removes the tenant publication but cannot delete Entra app registrations in the + // customer tenant, so it returns any it created for this server for the CLI to clean up here. + await CleanupEntraAppsAsync(logger, graphApiService, response.AppIdsToCleanup, serverName); + }, envIdOption, serverNameOption, dryRunOption, verboseOption); return command; } + /// + /// Best-effort deletion of the Entra app registrations the platform returned from an unpublish. Each + /// delete is independent so one failure does not skip the rest, and every failure is logged with the + /// app id so the user can remove it manually. When Graph is unavailable or the tenant cannot be + /// detected, the app ids are logged for manual cleanup instead. + /// + private static async Task CleanupEntraAppsAsync( + ILogger logger, + GraphApiService? graphApiService, + IReadOnlyList? appsToCleanup, + string serverName) + { + if (appsToCleanup is null || appsToCleanup.Count == 0) + { + return; + } + + if (graphApiService is null) + { + foreach (var app in appsToCleanup) + { + logger.LogWarning( + "Graph API is unavailable; cannot delete Entra app '{AppName}' (appId {AppId}) for server '{ServerName}'. Delete it manually in the Azure portal.", + app.AppName ?? "", app.AppId ?? "", serverName); + } + + return; + } + + var tenantId = await TenantDetectionHelper.DetectTenantIdAsync(null, logger); + if (string.IsNullOrWhiteSpace(tenantId)) + { + foreach (var app in appsToCleanup) + { + logger.LogWarning( + "Could not detect the tenant; cannot delete Entra app '{AppName}' (appId {AppId}) for server '{ServerName}'. Delete it manually in the Azure portal.", + app.AppName ?? "", app.AppId ?? "", serverName); + } + + return; + } + + logger.LogInformation("Cleaning up Entra app registrations for unpublished server '{ServerName}'...", serverName); + + foreach (var app in appsToCleanup) + { + if (string.IsNullOrWhiteSpace(app.AppId)) + { + continue; + } + + try + { + var objectId = await graphApiService.GetAppObjectIdByAppIdAsync(tenantId, app.AppId); + if (string.IsNullOrWhiteSpace(objectId)) + { + logger.LogWarning( + "Entra app '{AppName}' (appId {AppId}) was not found; it may already be deleted.", + app.AppName ?? "", app.AppId); + continue; + } + + var deleted = await graphApiService.DeleteEntraAppAsync(tenantId, objectId); + if (deleted) + { + logger.LogInformation("Deleted Entra app '{AppName}' (appId {AppId})", app.AppName ?? "", app.AppId); + } + else + { + logger.LogError( + "Failed to delete Entra app '{AppName}' (appId {AppId}). Delete it manually in the Azure portal.", + app.AppName ?? "", app.AppId); + } + } + catch (Exception ex) + { + logger.LogError( + ex, + "Exception deleting Entra app '{AppName}' (appId {AppId}). Delete it manually in the Azure portal.", + app.AppName ?? "", app.AppId); + } + } + } + /// /// Creates the register-external-mcp-server subcommand /// diff --git a/src/Microsoft.Agents.A365.DevTools.Cli/Models/McpServerAppEntry.cs b/src/Microsoft.Agents.A365.DevTools.Cli/Models/McpServerAppEntry.cs new file mode 100644 index 00000000..45276f61 --- /dev/null +++ b/src/Microsoft.Agents.A365.DevTools.Cli/Models/McpServerAppEntry.cs @@ -0,0 +1,26 @@ +// Copyright (c) Microsoft Corporation. +// Licensed under the MIT License. +using System.Text.Json.Serialization; + +namespace Microsoft.Agents.A365.DevTools.Cli.Models; + +/// +/// An Entra app registration associated with a published MCP server that the CLI is responsible for +/// deleting on unpublish. The platform cannot delete app registrations in the customer tenant, so it +/// returns these entries and the CLI performs the deletion using the caller's Graph permissions. +/// +public class McpServerAppEntry +{ + /// + /// Friendly name of the app registration (for logging / manual cleanup guidance). + /// + [JsonPropertyName("AppName")] + public string? AppName { get; set; } + + /// + /// The app (client) id of the Entra app registration. The CLI resolves the underlying application + /// object id from this before calling Graph's application delete. + /// + [JsonPropertyName("AppId")] + public string? AppId { get; set; } +} diff --git a/src/Microsoft.Agents.A365.DevTools.Cli/Models/UnpublishMcpServerResponse.cs b/src/Microsoft.Agents.A365.DevTools.Cli/Models/UnpublishMcpServerResponse.cs new file mode 100644 index 00000000..edf9d1f6 --- /dev/null +++ b/src/Microsoft.Agents.A365.DevTools.Cli/Models/UnpublishMcpServerResponse.cs @@ -0,0 +1,40 @@ +// Copyright (c) Microsoft Corporation. +// Licensed under the MIT License. +using System; +using System.Collections.Generic; +using System.Text.Json.Serialization; + +namespace Microsoft.Agents.A365.DevTools.Cli.Models; + +/// +/// Response model for an MCP server unpublish operation. +/// +public class UnpublishMcpServerResponse +{ + /// + /// Status of the unpublish operation. + /// + [JsonPropertyName("Status")] + public string? Status { get; set; } + + /// + /// Message from the API response. + /// + [JsonPropertyName("Message")] + public string? Message { get; set; } + + /// + /// Entra app registrations the platform created for this server that the CLI must delete: the + /// platform's identity cannot delete app registrations in the customer tenant, so it returns them + /// here for the CLI to clean up. Currently the server's Public Clients app; empty/null when the + /// server had none (for example OOB Dataverse servers or legacy records). + /// + [JsonPropertyName("AppIdsToCleanup")] + public List? AppIdsToCleanup { get; set; } + + /// + /// Whether the operation was successful. + /// + [JsonIgnore] + public bool IsSuccess => Status?.Equals("Success", StringComparison.OrdinalIgnoreCase) ?? false; +} diff --git a/src/Microsoft.Agents.A365.DevTools.Cli/Services/Agent365ToolingService.cs b/src/Microsoft.Agents.A365.DevTools.Cli/Services/Agent365ToolingService.cs index 578b0839..7c4a28fa 100644 --- a/src/Microsoft.Agents.A365.DevTools.Cli/Services/Agent365ToolingService.cs +++ b/src/Microsoft.Agents.A365.DevTools.Cli/Services/Agent365ToolingService.cs @@ -553,7 +553,7 @@ private string BuildProvisionIdentityUrl(string environment, string serverName) } /// - public async Task UnpublishServerAsync( + public async Task UnpublishServerAsync( string environmentId, string serverName, CancellationToken cancellationToken = default) @@ -587,7 +587,7 @@ public async Task UnpublishServerAsync( if (string.IsNullOrWhiteSpace(authToken)) { _logger.LogError("Failed to acquire authentication token"); - return false; + return null; } // Create authenticated HTTP client @@ -600,19 +600,37 @@ public async Task UnpublishServerAsync( using var response = await httpClient.DeleteAsync(endpointUrl, cancellationToken); // Validate response using common helper - var (isSuccess, _) = await ValidateResponseAsync(response, "unpublish MCP server", cancellationToken); + var (isSuccess, responseContent) = await ValidateResponseAsync(response, "unpublish MCP server", cancellationToken); if (!isSuccess) { - return false; + return null; } _logger.LogDebug("Successfully unpublished MCP server"); - return true; + + // Allow for an empty/null body: the operation still succeeded, there is just nothing to clean up. + if (string.IsNullOrWhiteSpace(responseContent)) + { + return new UnpublishMcpServerResponse + { + Status = "Success", + Message = $"Successfully unpublished {serverName}" + }; + } + + var unpublishResponse = JsonDeserializationHelper.DeserializeWithDoubleSerialization( + responseContent, _logger); + + return unpublishResponse ?? new UnpublishMcpServerResponse + { + Status = "Success", + Message = $"Successfully unpublished {serverName}" + }; } catch (Exception ex) { _logger.LogError(ex, "Failed to unpublish MCP server {ServerName} from environment {EnvId}", serverName, environmentId); - return false; + return null; } } diff --git a/src/Microsoft.Agents.A365.DevTools.Cli/Services/GraphApiService.cs b/src/Microsoft.Agents.A365.DevTools.Cli/Services/GraphApiService.cs index 8ed1388f..7c4f1585 100644 --- a/src/Microsoft.Agents.A365.DevTools.Cli/Services/GraphApiService.cs +++ b/src/Microsoft.Agents.A365.DevTools.Cli/Services/GraphApiService.cs @@ -603,6 +603,30 @@ public virtual async Task ApplicationExistsByAppIdAsync( return doc.RootElement.TryGetProperty("value", out var value) && value.GetArrayLength() > 0; } + /// + /// Resolves an application's object ID from its appId (client ID). Returns null when the application + /// cannot be found (for example it was already deleted). Virtual to allow substitution in unit tests. + /// + public virtual async Task GetAppObjectIdByAppIdAsync( + string tenantId, string appId, CancellationToken ct = default) + { + // Validate GUID format to prevent OData injection. + if (!Guid.TryParse(appId, out var validGuid)) + { + _logger.LogWarning("Invalid appId format for application lookup: {AppId}", appId); + return null; + } + + using var doc = await GraphGetAsync( + tenantId, + $"/v1.0/applications?$filter=appId eq '{validGuid:D}'&$select=id&$top=1", + ct); + if (doc == null) return null; + if (!doc.RootElement.TryGetProperty("value", out var value) || value.GetArrayLength() == 0) return null; + if (!value[0].TryGetProperty("id", out var id)) return null; + return id.GetString(); + } + /// /// Looks up the display name of a service principal by its application ID. /// Returns null if the service principal is not found. diff --git a/src/Microsoft.Agents.A365.DevTools.Cli/Services/IAgent365ToolingService.cs b/src/Microsoft.Agents.A365.DevTools.Cli/Services/IAgent365ToolingService.cs index 91ba94e6..63afa762 100644 --- a/src/Microsoft.Agents.A365.DevTools.Cli/Services/IAgent365ToolingService.cs +++ b/src/Microsoft.Agents.A365.DevTools.Cli/Services/IAgent365ToolingService.cs @@ -52,8 +52,11 @@ public interface IAgent365ToolingService /// Dataverse environment ID /// MCP server name to unpublish /// Cancellation token - /// True if successful, false otherwise - Task UnpublishServerAsync( + /// + /// The unpublish response (including any Entra app registrations the CLI must delete), or null when + /// the operation failed. + /// + Task UnpublishServerAsync( string environmentId, string serverName, CancellationToken cancellationToken = default); diff --git a/src/Tests/Microsoft.Agents.A365.DevTools.Cli.Tests/Commands/DevelopMcpCommandRegressionTests.cs b/src/Tests/Microsoft.Agents.A365.DevTools.Cli.Tests/Commands/DevelopMcpCommandRegressionTests.cs index 5d9a079d..30b8208c 100644 --- a/src/Tests/Microsoft.Agents.A365.DevTools.Cli.Tests/Commands/DevelopMcpCommandRegressionTests.cs +++ b/src/Tests/Microsoft.Agents.A365.DevTools.Cli.Tests/Commands/DevelopMcpCommandRegressionTests.cs @@ -72,7 +72,8 @@ public async Task AzureCliStyleParameters_AreAcceptedCorrectly(string command, p _mockToolingService.ListServersAsync(Arg.Any()).Returns(new DataverseMcpServersResponse()); _mockToolingService.PublishServerAsync(Arg.Any(), Arg.Any(), Arg.Any()) .Returns(new PublishMcpServerResponse { Status = "Success" }); - _mockToolingService.UnpublishServerAsync(Arg.Any(), Arg.Any()).Returns(true); + _mockToolingService.UnpublishServerAsync(Arg.Any(), Arg.Any()) + .Returns(new UnpublishMcpServerResponse { Status = "Success" }); var fullCommand = new List { command }; fullCommand.AddRange(args); @@ -336,7 +337,8 @@ public async Task ServiceIntegration_UnpublishCommand_PassesCorrectParameters() var testEnvId = "test-environment-456"; var testServerName = "msdyn_TestServer"; - _mockToolingService.UnpublishServerAsync(testEnvId, testServerName).Returns(true); + _mockToolingService.UnpublishServerAsync(testEnvId, testServerName) + .Returns(new UnpublishMcpServerResponse { Status = "Success" }); // Act var result = await _command.InvokeAsync(new[] @@ -351,6 +353,56 @@ public async Task ServiceIntegration_UnpublishCommand_PassesCorrectParameters() await _mockToolingService.Received(1).UnpublishServerAsync(testEnvId, testServerName); } + [Fact] + public async Task UnpublishCommand_WhenServiceReturnsNull_DoesNotThrow() + { + // A null response means the unpublish failed; the command must log and return cleanly. + var testEnvId = "test-environment-789"; + var testServerName = "msdyn_TestServer"; + + _mockToolingService.UnpublishServerAsync(testEnvId, testServerName) + .Returns((UnpublishMcpServerResponse?)null); + + var result = await _command.InvokeAsync(new[] + { + "unpublish", + "-e", testEnvId, + "-s", testServerName + }); + + result.Should().Be(0); + await _mockToolingService.Received(1).UnpublishServerAsync(testEnvId, testServerName); + } + + [Fact] + public async Task UnpublishCommand_WithAppsToCleanup_NoGraphService_SucceedsWithoutThrow() + { + // The command is created without a GraphApiService (the default), so the returned apps cannot be + // deleted. The cleanup step must degrade gracefully (warn, no throw) rather than fail the command. + var testEnvId = "test-environment-abc"; + var testServerName = "TestCustomServer"; + + _mockToolingService.UnpublishServerAsync(testEnvId, testServerName) + .Returns(new UnpublishMcpServerResponse + { + Status = "Success", + AppIdsToCleanup = new List + { + new() { AppName = $"{testServerName}-PublicClients", AppId = "11111111-1111-1111-1111-111111111111" }, + }, + }); + + var result = await _command.InvokeAsync(new[] + { + "unpublish", + "-e", testEnvId, + "-s", testServerName + }); + + result.Should().Be(0); + await _mockToolingService.Received(1).UnpublishServerAsync(testEnvId, testServerName); + } + [Fact] public void CommandStructure_HasNoPositionalArguments() { From b976c176fd4928261a60304ecde70a2657ad8ab9 Mon Sep 17 00:00:00 2001 From: Deepali Garg Date: Thu, 16 Jul 2026 16:39:06 -0700 Subject: [PATCH 2/4] Consume ManualCleanupRequired block on unpublish response Follow the platform's self-describing unpublish contract: read the Entra apps the platform cannot delete from response.ManualCleanupRequired.Apps instead of the removed AppIdsToCleanup list. Adds the mirroring McpServerManualCleanup model and updates the cleanup regression test. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> --- .../Commands/DevelopMcpCommand.cs | 5 ++-- .../Models/McpServerManualCleanup.cs | 27 +++++++++++++++++++ .../Models/UnpublishMcpServerResponse.cs | 12 ++++----- .../DevelopMcpCommandRegressionTests.cs | 8 ++++-- 4 files changed, 41 insertions(+), 11 deletions(-) create mode 100644 src/Microsoft.Agents.A365.DevTools.Cli/Models/McpServerManualCleanup.cs diff --git a/src/Microsoft.Agents.A365.DevTools.Cli/Commands/DevelopMcpCommand.cs b/src/Microsoft.Agents.A365.DevTools.Cli/Commands/DevelopMcpCommand.cs index 1b45a249..57033e05 100644 --- a/src/Microsoft.Agents.A365.DevTools.Cli/Commands/DevelopMcpCommand.cs +++ b/src/Microsoft.Agents.A365.DevTools.Cli/Commands/DevelopMcpCommand.cs @@ -530,8 +530,9 @@ private static Command CreateUnpublishSubcommand( logger.LogInformation("Successfully unpublished MCP server {ServerName} from environment {EnvId}", serverName, envId); // The platform removes the tenant publication but cannot delete Entra app registrations in the - // customer tenant, so it returns any it created for this server for the CLI to clean up here. - await CleanupEntraAppsAsync(logger, graphApiService, response.AppIdsToCleanup, serverName); + // customer tenant, so it returns any it created for this server (in ManualCleanupRequired) for + // the CLI to clean up here. + await CleanupEntraAppsAsync(logger, graphApiService, response.ManualCleanupRequired?.Apps, serverName); }, envIdOption, serverNameOption, dryRunOption, verboseOption); diff --git a/src/Microsoft.Agents.A365.DevTools.Cli/Models/McpServerManualCleanup.cs b/src/Microsoft.Agents.A365.DevTools.Cli/Models/McpServerManualCleanup.cs new file mode 100644 index 00000000..105ba654 --- /dev/null +++ b/src/Microsoft.Agents.A365.DevTools.Cli/Models/McpServerManualCleanup.cs @@ -0,0 +1,27 @@ +// Copyright (c) Microsoft Corporation. +// Licensed under the MIT License. +using System.Collections.Generic; +using System.Text.Json.Serialization; + +namespace Microsoft.Agents.A365.DevTools.Cli.Models; + +/// +/// Describes resources the unpublish operation could not delete on the caller's behalf and that the +/// caller must remove manually. Mirrors the platform's response contract so any client (not just this +/// CLI) can discover the cleanup responsibility from the response body. +/// +public class McpServerManualCleanup +{ + /// + /// Human-readable explanation of why the listed resources were not deleted automatically and how + /// to remove them. + /// + [JsonPropertyName("Reason")] + public string? Reason { get; set; } + + /// + /// The Entra app registrations the caller must delete manually in their own tenant. + /// + [JsonPropertyName("Apps")] + public List? Apps { get; set; } +} diff --git a/src/Microsoft.Agents.A365.DevTools.Cli/Models/UnpublishMcpServerResponse.cs b/src/Microsoft.Agents.A365.DevTools.Cli/Models/UnpublishMcpServerResponse.cs index edf9d1f6..8c7ad57b 100644 --- a/src/Microsoft.Agents.A365.DevTools.Cli/Models/UnpublishMcpServerResponse.cs +++ b/src/Microsoft.Agents.A365.DevTools.Cli/Models/UnpublishMcpServerResponse.cs @@ -1,7 +1,6 @@ // Copyright (c) Microsoft Corporation. // Licensed under the MIT License. using System; -using System.Collections.Generic; using System.Text.Json.Serialization; namespace Microsoft.Agents.A365.DevTools.Cli.Models; @@ -24,13 +23,12 @@ public class UnpublishMcpServerResponse public string? Message { get; set; } /// - /// Entra app registrations the platform created for this server that the CLI must delete: the - /// platform's identity cannot delete app registrations in the customer tenant, so it returns them - /// here for the CLI to clean up. Currently the server's Public Clients app; empty/null when the - /// server had none (for example OOB Dataverse servers or legacy records). + /// Resources the platform could not delete in the customer tenant and that the caller must remove + /// manually (with a reason and the affected app registrations). Null when there is nothing for the + /// caller to clean up (for example OOB Dataverse servers or legacy records). /// - [JsonPropertyName("AppIdsToCleanup")] - public List? AppIdsToCleanup { get; set; } + [JsonPropertyName("ManualCleanupRequired")] + public McpServerManualCleanup? ManualCleanupRequired { get; set; } /// /// Whether the operation was successful. diff --git a/src/Tests/Microsoft.Agents.A365.DevTools.Cli.Tests/Commands/DevelopMcpCommandRegressionTests.cs b/src/Tests/Microsoft.Agents.A365.DevTools.Cli.Tests/Commands/DevelopMcpCommandRegressionTests.cs index 30b8208c..f7027287 100644 --- a/src/Tests/Microsoft.Agents.A365.DevTools.Cli.Tests/Commands/DevelopMcpCommandRegressionTests.cs +++ b/src/Tests/Microsoft.Agents.A365.DevTools.Cli.Tests/Commands/DevelopMcpCommandRegressionTests.cs @@ -386,9 +386,13 @@ public async Task UnpublishCommand_WithAppsToCleanup_NoGraphService_SucceedsWith .Returns(new UnpublishMcpServerResponse { Status = "Success", - AppIdsToCleanup = new List + ManualCleanupRequired = new McpServerManualCleanup { - new() { AppName = $"{testServerName}-PublicClients", AppId = "11111111-1111-1111-1111-111111111111" }, + Reason = "The platform cannot delete Entra app registrations in your tenant.", + Apps = new List + { + new() { AppName = $"{testServerName}-PublicClients", AppId = "11111111-1111-1111-1111-111111111111" }, + }, }, }); From 4ea88b4dd3bfb5d8433da756956697bcda9af52e Mon Sep 17 00:00:00 2001 From: Deepali Garg Date: Tue, 21 Jul 2026 10:50:51 -0700 Subject: [PATCH 3/4] Fixing PR comments --- .../Commands/DevelopMcpCommand.cs | 19 +++++++--- .../Services/Agent365ToolingService.cs | 21 ++++++++--- .../Services/GraphApiService.cs | 36 ------------------- .../DevelopMcpCommandRegressionTests.cs | 7 ++-- 4 files changed, 35 insertions(+), 48 deletions(-) diff --git a/src/Microsoft.Agents.A365.DevTools.Cli/Commands/DevelopMcpCommand.cs b/src/Microsoft.Agents.A365.DevTools.Cli/Commands/DevelopMcpCommand.cs index 57033e05..b8f3430c 100644 --- a/src/Microsoft.Agents.A365.DevTools.Cli/Commands/DevelopMcpCommand.cs +++ b/src/Microsoft.Agents.A365.DevTools.Cli/Commands/DevelopMcpCommand.cs @@ -456,9 +456,12 @@ private static Command CreateUnpublishSubcommand( ); command.AddOption(verboseOption); - command.SetHandler(async (envId, serverName, dryRun, verbose) => + command.SetHandler(async (context) => { - _ = verbose; + var envId = context.ParseResult.GetValueForOption(envIdOption); + var serverName = context.ParseResult.GetValueForOption(serverNameOption); + var dryRun = context.ParseResult.GetValueForOption(dryRunOption); + try { // Validate and prompt for missing required arguments with security checks @@ -468,6 +471,7 @@ private static Command CreateUnpublishSubcommand( if (string.IsNullOrWhiteSpace(envId)) { logger.LogError("Environment ID is required"); + context.ExitCode = 1; return; } } @@ -478,6 +482,7 @@ private static Command CreateUnpublishSubcommand( if (envId == null) { logger.LogError("Invalid environment ID format"); + context.ExitCode = 1; return; } } @@ -488,6 +493,7 @@ private static Command CreateUnpublishSubcommand( if (string.IsNullOrWhiteSpace(serverName)) { logger.LogError("Server name is required"); + context.ExitCode = 1; return; } } @@ -498,6 +504,7 @@ private static Command CreateUnpublishSubcommand( if (serverName == null) { logger.LogError("Invalid server name format"); + context.ExitCode = 1; return; } } @@ -505,6 +512,7 @@ private static Command CreateUnpublishSubcommand( catch (ArgumentException ex) { logger.LogError("Input validation failed: {Message}", ex.Message); + context.ExitCode = 1; return; } @@ -524,6 +532,7 @@ private static Command CreateUnpublishSubcommand( if (response is null || !response.IsSuccess) { logger.LogError("Failed to unpublish MCP server {ServerName} from environment {EnvId}", serverName, envId); + context.ExitCode = 1; return; } @@ -533,8 +542,7 @@ private static Command CreateUnpublishSubcommand( // customer tenant, so it returns any it created for this server (in ManualCleanupRequired) for // the CLI to clean up here. await CleanupEntraAppsAsync(logger, graphApiService, response.ManualCleanupRequired?.Apps, serverName); - - }, envIdOption, serverNameOption, dryRunOption, verboseOption); + }); return command; } @@ -587,6 +595,9 @@ private static async Task CleanupEntraAppsAsync( { if (string.IsNullOrWhiteSpace(app.AppId)) { + logger.LogWarning( + "The platform returned Entra app '{AppName}' for cleanup without an appId, so it cannot be deleted automatically. If it exists, delete it manually in the Azure portal.", + app.AppName ?? ""); continue; } diff --git a/src/Microsoft.Agents.A365.DevTools.Cli/Services/Agent365ToolingService.cs b/src/Microsoft.Agents.A365.DevTools.Cli/Services/Agent365ToolingService.cs index 7c4a28fa..197ce6de 100644 --- a/src/Microsoft.Agents.A365.DevTools.Cli/Services/Agent365ToolingService.cs +++ b/src/Microsoft.Agents.A365.DevTools.Cli/Services/Agent365ToolingService.cs @@ -621,11 +621,22 @@ private string BuildProvisionIdentityUrl(string environment, string serverName) var unpublishResponse = JsonDeserializationHelper.DeserializeWithDoubleSerialization( responseContent, _logger); - return unpublishResponse ?? new UnpublishMcpServerResponse - { - Status = "Success", - Message = $"Successfully unpublished {serverName}" - }; + // A non-empty body that fails to parse is not fatal to the unpublish itself, but it means we + // may have lost the ManualCleanupRequired block - so warn the user to check for leftovers rather + // than silently returning a clean success. + if (unpublishResponse is null) + { + _logger.LogWarning( + "The unpublish for {ServerName} succeeded but its response body could not be parsed, so any Entra app registrations the platform asked to be cleaned up may have been missed. Review the server's app registrations in the Azure portal and delete any leftovers manually.", + serverName); + return new UnpublishMcpServerResponse + { + Status = "Success", + Message = $"Successfully unpublished {serverName}" + }; + } + + return unpublishResponse; } catch (Exception ex) { diff --git a/src/Microsoft.Agents.A365.DevTools.Cli/Services/GraphApiService.cs b/src/Microsoft.Agents.A365.DevTools.Cli/Services/GraphApiService.cs index 7c4f1585..cfa46557 100644 --- a/src/Microsoft.Agents.A365.DevTools.Cli/Services/GraphApiService.cs +++ b/src/Microsoft.Agents.A365.DevTools.Cli/Services/GraphApiService.cs @@ -2269,42 +2269,6 @@ public virtual async Task UpdateAppPublicClientRedirectUrisAsync( return result; } - /// - /// Looks up an application by its appId (clientId) and returns the object ID. - /// Retries up to 6 times with a 10-second delay to handle replication lag for newly created apps. - /// - public virtual async Task GetAppObjectIdByClientIdAsync( - string tenantId, string clientId, CancellationToken ct = default) - { - const int maxAttempts = 6; - const int delayMs = 10_000; - - for (var attempt = 0; attempt < maxAttempts; attempt++) - { - var response = await GraphGetWithResponseAsync(tenantId, $"/v1.0/applications?$filter=appId eq '{clientId}'&$select=id", ct: ct); - if (response.IsSuccess && response.Json != null) - { - var values = response.Json.RootElement.GetProperty("value"); - if (values.GetArrayLength() > 0) - { - return values[0].GetProperty("id").GetString(); - } - } - else - { - _logger.LogDebug("App {ClientId} query failed: {Code} {Reason} (attempt {Attempt}/{Max})", clientId, response.StatusCode, response.ReasonPhrase, attempt + 1, maxAttempts); - } - - if (attempt < maxAttempts - 1) - { - _logger.LogDebug("App {ClientId} not found yet, retrying in {Delay}s (attempt {Attempt}/{Max})...", clientId, delayMs / 1000, attempt + 1, maxAttempts); - await Task.Delay(delayMs, ct); - } - } - - return null; - } - /// /// Finds an application's object ID by its display name. /// diff --git a/src/Tests/Microsoft.Agents.A365.DevTools.Cli.Tests/Commands/DevelopMcpCommandRegressionTests.cs b/src/Tests/Microsoft.Agents.A365.DevTools.Cli.Tests/Commands/DevelopMcpCommandRegressionTests.cs index f7027287..7df179c2 100644 --- a/src/Tests/Microsoft.Agents.A365.DevTools.Cli.Tests/Commands/DevelopMcpCommandRegressionTests.cs +++ b/src/Tests/Microsoft.Agents.A365.DevTools.Cli.Tests/Commands/DevelopMcpCommandRegressionTests.cs @@ -354,9 +354,10 @@ public async Task ServiceIntegration_UnpublishCommand_PassesCorrectParameters() } [Fact] - public async Task UnpublishCommand_WhenServiceReturnsNull_DoesNotThrow() + public async Task UnpublishCommand_WhenServiceReturnsNull_ReturnsNonZeroExitCode() { - // A null response means the unpublish failed; the command must log and return cleanly. + // A null response means the unpublish failed; the command must log the failure and surface it to + // the shell as a non-zero exit code (not swallow it as success). var testEnvId = "test-environment-789"; var testServerName = "msdyn_TestServer"; @@ -370,7 +371,7 @@ public async Task UnpublishCommand_WhenServiceReturnsNull_DoesNotThrow() "-s", testServerName }); - result.Should().Be(0); + result.Should().Be(1); await _mockToolingService.Received(1).UnpublishServerAsync(testEnvId, testServerName); } From d49f91a60246746ddd186172e1b47c60c418e95f Mon Sep 17 00:00:00 2001 From: Deepali Garg Date: Tue, 21 Jul 2026 15:35:37 -0700 Subject: [PATCH 4/4] Addressing comments --- .../Commands/DevelopMcpCommand.cs | 34 +++++++------------ .../Services/Agent365ToolingService.cs | 25 +++++--------- 2 files changed, 22 insertions(+), 37 deletions(-) diff --git a/src/Microsoft.Agents.A365.DevTools.Cli/Commands/DevelopMcpCommand.cs b/src/Microsoft.Agents.A365.DevTools.Cli/Commands/DevelopMcpCommand.cs index b8f3430c..cabaa04b 100644 --- a/src/Microsoft.Agents.A365.DevTools.Cli/Commands/DevelopMcpCommand.cs +++ b/src/Microsoft.Agents.A365.DevTools.Cli/Commands/DevelopMcpCommand.cs @@ -7,6 +7,7 @@ using Microsoft.Agents.A365.DevTools.Cli.Services.Evaluate; using Microsoft.Extensions.Logging; using System.CommandLine; +using System.Linq; namespace Microsoft.Agents.A365.DevTools.Cli.Commands; @@ -551,7 +552,7 @@ private static Command CreateUnpublishSubcommand( /// Best-effort deletion of the Entra app registrations the platform returned from an unpublish. Each /// delete is independent so one failure does not skip the rest, and every failure is logged with the /// app id so the user can remove it manually. When Graph is unavailable or the tenant cannot be - /// detected, the app ids are logged for manual cleanup instead. + /// detected, the apps are listed once for manual cleanup instead. /// private static async Task CleanupEntraAppsAsync( ILogger logger, @@ -564,28 +565,19 @@ private static async Task CleanupEntraAppsAsync( return; } - if (graphApiService is null) - { - foreach (var app in appsToCleanup) - { - logger.LogWarning( - "Graph API is unavailable; cannot delete Entra app '{AppName}' (appId {AppId}) for server '{ServerName}'. Delete it manually in the Azure portal.", - app.AppName ?? "", app.AppId ?? "", serverName); - } + // Tenant detection shells out to az, so only attempt it when Graph is available. + var tenantId = graphApiService is null + ? null + : await TenantDetectionHelper.DetectTenantIdAsync(null, logger); - return; - } - - var tenantId = await TenantDetectionHelper.DetectTenantIdAsync(null, logger); - if (string.IsNullOrWhiteSpace(tenantId)) + if (graphApiService is null || string.IsNullOrWhiteSpace(tenantId)) { - foreach (var app in appsToCleanup) - { - logger.LogWarning( - "Could not detect the tenant; cannot delete Entra app '{AppName}' (appId {AppId}) for server '{ServerName}'. Delete it manually in the Azure portal.", - app.AppName ?? "", app.AppId ?? "", serverName); - } - + var appList = string.Join( + Environment.NewLine, + appsToCleanup.Select(app => $" - {app.AppName ?? ""} (appId {app.AppId ?? ""})")); + logger.LogWarning( + "The platform could not automatically delete {Count} Entra app registration(s) for server '{ServerName}'. Delete them manually in the Azure portal:{NewLine}{AppList}", + appsToCleanup.Count, serverName, Environment.NewLine, appList); return; } diff --git a/src/Microsoft.Agents.A365.DevTools.Cli/Services/Agent365ToolingService.cs b/src/Microsoft.Agents.A365.DevTools.Cli/Services/Agent365ToolingService.cs index 197ce6de..edcd4965 100644 --- a/src/Microsoft.Agents.A365.DevTools.Cli/Services/Agent365ToolingService.cs +++ b/src/Microsoft.Agents.A365.DevTools.Cli/Services/Agent365ToolingService.cs @@ -608,26 +608,19 @@ private string BuildProvisionIdentityUrl(string environment, string serverName) _logger.LogDebug("Successfully unpublished MCP server"); - // Allow for an empty/null body: the operation still succeeded, there is just nothing to clean up. - if (string.IsNullOrWhiteSpace(responseContent)) - { - return new UnpublishMcpServerResponse - { - Status = "Success", - Message = $"Successfully unpublished {serverName}" - }; - } - - var unpublishResponse = JsonDeserializationHelper.DeserializeWithDoubleSerialization( - responseContent, _logger); + var unpublishResponse = string.IsNullOrWhiteSpace(responseContent) + ? null + : JsonDeserializationHelper.DeserializeWithDoubleSerialization( + responseContent, _logger); - // A non-empty body that fails to parse is not fatal to the unpublish itself, but it means we - // may have lost the ManualCleanupRequired block - so warn the user to check for leftovers rather - // than silently returning a clean success. + // The platform always returns a JSON body on a successful unpublish, so an empty body or one + // that fails to parse means we could not read the response - including any ManualCleanupRequired + // block. The unpublish itself still succeeded, so warn the user to check for leftover Entra app + // registrations rather than silently returning a clean success. if (unpublishResponse is null) { _logger.LogWarning( - "The unpublish for {ServerName} succeeded but its response body could not be parsed, so any Entra app registrations the platform asked to be cleaned up may have been missed. Review the server's app registrations in the Azure portal and delete any leftovers manually.", + "The unpublish for {ServerName} succeeded but its response body was empty or could not be parsed, so any Entra app registrations the platform asked to be cleaned up may have been missed. Review the server's app registrations in the Azure portal and delete any leftovers manually.", serverName); return new UnpublishMcpServerResponse {