Add manual cleanup block for unpublish by deepaligargms · Pull Request #477 · microsoft/Agent365-devTools

deepaligargms · 2026-07-17T20:12:57Z

Summary

When you unpublish a custom MCP server, the platform removes the tenant publication (MOS title + MCC record) but cannot delete the Entra app registration it created for the server (the *-PublicClients app), because the platform's identity has no rights to delete app registrations in the customer tenant. Previously that app was left dangling after unpublish, with nothing telling the user it existed.

The platform now returns those apps on the unpublish response under a self-describing ManualCleanupRequired block. This PR teaches the CLI to consume that block and delete the apps on the user's behalf — mirroring the rollback the publish flow already does when a publish fails.

What changed

UnpublishServerAsync now returns the parsed response (Task<UnpublishMcpServerResponse?> instead of Task), so the command layer can act on the returned apps. Returns null on failure.
New models mirroring the platform contract:

UnpublishMcpServerResponse — Status, Message, ManualCleanupRequired, IsSuccess.
McpServerManualCleanup — { Reason, Apps[] }.
McpServerAppEntry — { AppName, AppId }.
GraphApiService.GetAppObjectIdByAppIdAsync — resolves an app's object id from its app (client) id, since the platform returns the client id but Graph's delete needs the object id.
develop mcp unpublish now takes the GraphApiService and, after a successful unpublish, deletes each returned app best-effort: each delete is independent, failures are logged with the app id, and the step degrades gracefully (warn, don't fail the command) when Graph or the tenant can't be resolved — falling back to a manual-cleanup message so the user still knows what to remove.

Testing

CLI + test projects build clean (warnings-as-errors).
Full suite: 1923 passed / 0 failed / 12 skipped.
Added coverage: null-response path (unpublish failed → no throw) and the no-Graph-service cleanup path (degrades gracefully).

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>

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>

github-actions · 2026-07-17T20:13:53Z

⚠️ Deprecation Warning: The deny-licenses option is deprecated for possible removal in the next major release. For more information, see issue 997.

Dependency Review

✅ No vulnerabilities or license issues or OpenSSF Scorecard issues found.

Scanned Files

None

Copilot

Pull request overview

This PR updates the develop mcp unpublish flow to consume the platform’s new ManualCleanupRequired response block and (best-effort) delete Entra app registrations that the platform cannot remove on its own.

Changes:

Change UnpublishServerAsync to return a parsed UnpublishMcpServerResponse? (instead of bool) so the command layer can act on returned cleanup artifacts.
Add response/cleanup models (UnpublishMcpServerResponse, McpServerManualCleanup, McpServerAppEntry) mirroring the platform contract.
Add Graph support to resolve an app object id from an app (client) id and use it to delete returned apps after a successful unpublish.

Reviewed changes

Copilot reviewed 8 out of 8 changed files in this pull request and generated 4 comments.

Show a summary per file

File	Description
src/Tests/Microsoft.Agents.A365.DevTools.Cli.Tests/Commands/DevelopMcpCommandRegressionTests.cs	Updates mocks for new unpublish return type; adds regression tests for null response and no-Graph cleanup behavior.
src/Microsoft.Agents.A365.DevTools.Cli/Services/IAgent365ToolingService.cs	Updates unpublish service contract to return `UnpublishMcpServerResponse?`.
src/Microsoft.Agents.A365.DevTools.Cli/Services/GraphApiService.cs	Adds appId→objectId lookup helper for later deletion.
src/Microsoft.Agents.A365.DevTools.Cli/Services/Agent365ToolingService.cs	Parses unpublish response body (when present) and returns cleanup info to the command layer.
src/Microsoft.Agents.A365.DevTools.Cli/Models/UnpublishMcpServerResponse.cs	New unpublish response model (Status/Message/ManualCleanupRequired + `IsSuccess`).
src/Microsoft.Agents.A365.DevTools.Cli/Models/McpServerManualCleanup.cs	New model for the `ManualCleanupRequired` block.
src/Microsoft.Agents.A365.DevTools.Cli/Models/McpServerAppEntry.cs	New model for Entra app registrations returned for cleanup.
src/Microsoft.Agents.A365.DevTools.Cli/Commands/DevelopMcpCommand.cs	Wires unpublish to consume cleanup block and invoke best-effort Entra app deletion.

Comments suppressed due to low confidence (1)

src/Microsoft.Agents.A365.DevTools.Cli/Commands/DevelopMcpCommand.cs:528

The unpublish handler logs an error and returns on failure, but it never sets a non-zero exit code. As a result, a365 develop mcp unpublish can fail (null/!IsSuccess response) while still exiting 0, which breaks scripting/CI usage and conflicts with the repo’s command exit-code rule for failure branches.

            var response = await toolingService.UnpublishServerAsync(envId, serverName);

            if (response is null || !response.IsSuccess)
            {
                logger.LogError("Failed to unpublish MCP server {ServerName} from environment {EnvId}", serverName, envId);
                return;
            }

+            if (string.IsNullOrWhiteSpace(app.AppId))
+            {
+                continue;
+            }


+            return unpublishResponse ?? new UnpublishMcpServerResponse
+            {
+                Status = "Success",
+                Message = $"Successfully unpublished {serverName}"
+            };


+    [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);
+    }


rbrighenti

Copilot has a few relevant comments that I think should be addressed. Please take a look at them

Copilot

Pull request overview

Copilot reviewed 8 out of 8 changed files in this pull request and generated 3 comments.

+            return;
+        }
+
+        var tenantId = await TenantDetectionHelper.DetectTenantIdAsync(null, logger);


ragurubhaarath · 2026-07-21T18:08:13Z

+            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.",


Remove "Graph API is unavailable"

ragurubhaarath · 2026-07-21T18:11:19Z

+            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.",


Also, may be log the message just once and then a list of apps?

ragurubhaarath · 2026-07-21T18:12:04Z

+            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.",


Same comment. May be check these two conditions first and then show the app list? User do not need know the internal details.

ragurubhaarath · 2026-07-21T18:27:52Z

-    /// 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.
-    /// </summary>
-    public virtual async Task<string?> GetAppObjectIdByClientIdAsync(


Is this deadcode?

ragurubhaarath · 2026-07-21T18:29:44Z

            _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.


This is not true, right? Is there a success case where the body would be empty?

deepaligargms and others added 2 commits July 16, 2026 15:59

Copilot AI review requested due to automatic review settings July 17, 2026 20:12

deepaligargms requested review from a team as code owners July 17, 2026 20:12

Copilot started reviewing on behalf of deepaligargms July 17, 2026 20:14 View session

Copilot AI reviewed Jul 17, 2026

View reviewed changes

rbrighenti reviewed Jul 20, 2026

View reviewed changes

Fixing PR comments

5a40969

Copilot AI review requested due to automatic review settings July 21, 2026 17:51

Copilot started reviewing on behalf of deepaligargms July 21, 2026 17:51 View session

Copilot AI reviewed Jul 21, 2026

View reviewed changes

ragurubhaarath reviewed Jul 21, 2026

View reviewed changes

ragurubhaarath requested changes Jul 21, 2026

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Add manual cleanup block for unpublish#477

Add manual cleanup block for unpublish#477
deepaligargms wants to merge 3 commits into
mainfrom
u/deepaligarg/unpublishchanges

deepaligargms commented Jul 17, 2026

Uh oh!

github-actions Bot commented Jul 17, 2026

Uh oh!

Copilot AI left a comment

Uh oh!

Uh oh!

rbrighenti left a comment

Uh oh!

Copilot AI left a comment

Uh oh!

Uh oh!

Uh oh!

ragurubhaarath Jul 21, 2026

Uh oh!

ragurubhaarath Jul 21, 2026

Uh oh!

ragurubhaarath Jul 21, 2026

Uh oh!

ragurubhaarath Jul 21, 2026

Uh oh!

ragurubhaarath Jul 21, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

Uh oh!

Conversation

deepaligargms commented Jul 17, 2026

Summary

What changed

Testing

Uh oh!

github-actions Bot commented Jul 17, 2026

Dependency Review

Scanned Files

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

Uh oh!

rbrighenti left a comment

Choose a reason for hiding this comment

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Uh oh!

Uh oh!

ragurubhaarath Jul 21, 2026

Choose a reason for hiding this comment

Uh oh!

ragurubhaarath Jul 21, 2026

Choose a reason for hiding this comment

Uh oh!

ragurubhaarath Jul 21, 2026

Choose a reason for hiding this comment

Uh oh!

ragurubhaarath Jul 21, 2026

Choose a reason for hiding this comment

Uh oh!

ragurubhaarath Jul 21, 2026

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants