fix: cleanup agent instances in a365 cleanup blueprint#284
fix: cleanup agent instances in a365 cleanup blueprint#284
Conversation
Dependency Review✅ No vulnerabilities or license issues or OpenSSF Scorecard issues found.Scanned FilesNone |
There was a problem hiding this comment.
Pull request overview
This pull request enhances the blueprint cleanup process in the Agent365 DevTools CLI by implementing cascading deletion of agent instances and improving error handling for partial failures. When cleaning up a blueprint, the CLI now queries for all agent instances linked to that blueprint, presents a detailed preview of resources to be deleted (including agentic users and identity service principals), and deletes these resources before deleting the blueprint itself. If any deletions fail, the user receives a clear summary of orphaned resources that need manual cleanup.
Changes:
- Added
AgentInstanceInfomodel to represent agent instances with their identity SP and optional agentic user - Enhanced
AgentBlueprintServicewith methods to query and delete agent instances and users, with virtual modifiers for testability - Updated blueprint cleanup command to cascade delete instances, use
IConfirmationProviderfor user confirmation, and provide detailed preview and orphan summaries - Added comprehensive test coverage for the new cascading deletion behavior and proper resource disposal in test infrastructure
Reviewed changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| src/Microsoft.Agents.A365.DevTools.Cli/Models/AgentInstanceInfo.cs | New record to represent agent instances linked to a blueprint with identity SP ID, display name, and optional agentic user ID |
| src/Microsoft.Agents.A365.DevTools.Cli/Services/AgentBlueprintService.cs | Added methods to query agent instances, delete agentic users, and made deletion methods virtual for testability; includes placeholder API property names that need verification |
| src/Microsoft.Agents.A365.DevTools.Cli/Commands/CleanupCommand.cs | Enhanced blueprint cleanup to cascade delete instances with preview, confirmation, failure tracking, and orphan summaries; now uses IConfirmationProvider |
| src/Tests/Microsoft.Agents.A365.DevTools.Cli.Tests/Services/AgentBlueprintServiceTests.cs | Added tests for querying and deleting agent instances/users; added Dispose implementation to FakeHttpMessageHandler for proper resource cleanup |
| src/Tests/Microsoft.Agents.A365.DevTools.Cli.Tests/Commands/CleanupCommandTests.cs | Added tests for cascading deletion behavior with instances, no instances, and partial failures; updated all mock calls to include optional correlationId parameter |
| src/Tests/Microsoft.Agents.A365.DevTools.Cli.Tests/Commands/CleanupCommandBotEndpointTests.cs | Updated mock setup to include optional correlationId parameter for endpoint deletion |
| src/Tests/Microsoft.Agents.A365.DevTools.Cli.Tests/Commands/BlueprintSubcommandTests.cs | Updated all mock calls and assertions to include optional correlationId parameter for endpoint deletion |
src/Microsoft.Agents.A365.DevTools.Cli/Services/AgentBlueprintService.cs
Outdated
Show resolved
Hide resolved
src/Tests/Microsoft.Agents.A365.DevTools.Cli.Tests/Commands/CleanupCommandTests.cs
Show resolved
Hide resolved
src/Tests/Microsoft.Agents.A365.DevTools.Cli.Tests/Services/AgentBlueprintServiceTests.cs
Outdated
Show resolved
Hide resolved
src/Tests/Microsoft.Agents.A365.DevTools.Cli.Tests/Services/AgentBlueprintServiceTests.cs
Outdated
Show resolved
Hide resolved
src/Tests/Microsoft.Agents.A365.DevTools.Cli.Tests/Services/AgentBlueprintServiceTests.cs
Outdated
Show resolved
Hide resolved
src/Tests/Microsoft.Agents.A365.DevTools.Cli.Tests/Services/AgentBlueprintServiceTests.cs
Outdated
Show resolved
Hide resolved
src/Tests/Microsoft.Agents.A365.DevTools.Cli.Tests/Services/AgentBlueprintServiceTests.cs
Outdated
Show resolved
Hide resolved
src/Tests/Microsoft.Agents.A365.DevTools.Cli.Tests/Services/AgentBlueprintServiceTests.cs
Show resolved
Hide resolved
src/Microsoft.Agents.A365.DevTools.Cli/Commands/CleanupCommand.cs
Outdated
Show resolved
Hide resolved
src/Microsoft.Agents.A365.DevTools.Cli/Services/AgentBlueprintService.cs
Outdated
Show resolved
Hide resolved
src/Tests/Microsoft.Agents.A365.DevTools.Cli.Tests/Services/AgentBlueprintServiceTests.cs
Outdated
Show resolved
Hide resolved
src/Microsoft.Agents.A365.DevTools.Cli/Services/AgentBlueprintService.cs
Outdated
Show resolved
Hide resolved
src/Tests/Microsoft.Agents.A365.DevTools.Cli.Tests/Services/AgentBlueprintServiceTests.cs
Outdated
Show resolved
Hide resolved
src/Tests/Microsoft.Agents.A365.DevTools.Cli.Tests/Services/AgentBlueprintServiceTests.cs
Outdated
Show resolved
Hide resolved
src/Tests/Microsoft.Agents.A365.DevTools.Cli.Tests/Services/AgentBlueprintServiceTests.cs
Outdated
Show resolved
Hide resolved
src/Tests/Microsoft.Agents.A365.DevTools.Cli.Tests/Services/AgentBlueprintServiceTests.cs
Outdated
Show resolved
Hide resolved
|
@gwharris7 I've opened a new pull request, #297, to work on those changes. Once the pull request is ready, I'll request review from you. |
…ts (#297) * Initial plan * Fix FakeHttpMessageHandler disposal to use using statements Co-authored-by: gwharris7 <96964444+gwharris7@users.noreply.github.com> --------- Co-authored-by: copilot-swe-agent[bot] <198982749+Copilot@users.noreply.github.com> Co-authored-by: gwharris7 <96964444+gwharris7@users.noreply.github.com>
src/Microsoft.Agents.A365.DevTools.Cli/Services/AgentBlueprintService.cs
Outdated
Show resolved
Hide resolved
src/Microsoft.Agents.A365.DevTools.Cli/Commands/CleanupCommand.cs
Outdated
Show resolved
Hide resolved
src/Tests/Microsoft.Agents.A365.DevTools.Cli.Tests/Services/AgentBlueprintServiceTests.cs
Show resolved
Hide resolved
src/Tests/Microsoft.Agents.A365.DevTools.Cli.Tests/Services/AgentBlueprintServiceTests.cs
Show resolved
Hide resolved
src/Microsoft.Agents.A365.DevTools.Cli/Services/AgentBlueprintService.cs
Outdated
Show resolved
Hide resolved
…Service.cs Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
…Service.cs Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
src/Microsoft.Agents.A365.DevTools.Cli/Services/AgentBlueprintService.cs
Show resolved
Hide resolved
src/Microsoft.Agents.A365.DevTools.Cli/Services/AgentBlueprintService.cs
Outdated
Show resolved
Hide resolved
| requiredScopes); | ||
|
|
||
| if (doc is null) | ||
| { |
There was a problem hiding this comment.
FetchAllPagesAsync silently breaks when GraphGetAsync returns null, which can yield incomplete/empty instance lists without any indication at the call site (GraphGetAsync also returns null on non-success). Consider logging a warning/error (including requestPath/status if available) when doc is null so cleanup preview doesn't misleadingly show zero instances on transient Graph failures.
| { | |
| { | |
| _logger.LogWarning( | |
| "Graph GET returned null for tenant {TenantId} on path {RequestPath}. " + | |
| "Stopping pagination and returning {ItemCount} items. This may indicate a transient Graph failure.", | |
| tenantId, | |
| requestPath, | |
| items.Count); |
| // Handle endpoint deletion if needed using shared helper | ||
| if (!await DeleteMessagingEndpointAsync(logger, config, botConfigurator, correlationId: correlationId)) | ||
| { | ||
| return; | ||
| } | ||
|
|
There was a problem hiding this comment.
If messaging endpoint deletion fails, the handler returns early, so any previously recorded instance deletion failures won't be summarized for the user. Consider calling PrintOrphanSummary before returning (or moving orphan reporting into a finally) so orphaned instance resources are always reported regardless of endpoint deletion outcome.
| Console.WriteLine(); | ||
| logger.LogWarning("Blueprint cleanup completed with warnings."); | ||
| logger.LogWarning("The following resources could not be deleted and remain orphaned in Entra ID:"); | ||
| foreach (var userId in failedResources[AgenticUsersKey]) | ||
| logger.LogWarning(" Orphaned agentic user: {ResourceId}", userId); | ||
| foreach (var spId in failedResources[IdentitySpsKey]) | ||
| logger.LogWarning(" Orphaned identity SP: {ResourceId}", spId); | ||
| logger.LogWarning("Delete them manually via the Entra portal or Graph API."); |
There was a problem hiding this comment.
PrintOrphanSummary always logs "Blueprint cleanup completed with warnings" and labels failed deletions as "orphaned". This method is also called when blueprint deletion fails, where cleanup did not complete and the remaining resources may not be orphaned (the blueprint still exists). Consider adjusting the wording based on whether the blueprint delete succeeded, or pass an explicit state flag into PrintOrphanSummary.
| // Response 2: Users query returns empty (both run in parallel) | ||
| handler.QueueResponse(new HttpResponseMessage(HttpStatusCode.OK) | ||
| { | ||
| Content = new StringContent(JsonSerializer.Serialize(new { value = Array.Empty<object>() })) | ||
| }); |
There was a problem hiding this comment.
This test comment says the two Graph queries "run in parallel", but GetAgentInstancesForBlueprintAsync now fetches SPs and users sequentially to avoid header races. Update the comment to match the current behavior so it doesn't mislead future maintainers.
| protected override void Dispose(bool disposing) | ||
| { | ||
| if (disposing) | ||
| { | ||
| while (_responses.Count > 0) | ||
| _responses.Dequeue().Dispose(); |
There was a problem hiding this comment.
FakeHttpMessageHandler.Dispose only disposes HttpResponseMessage instances still in the queue. Any responses already dequeued in SendAsync (notably via GraphGetAsync, which doesn't dispose its responses) will not be disposed, so these tests can still leak HttpResponseMessage objects. Consider tracking all created/dequeued responses and disposing them in Dispose, or updating GraphGetAsync to dispose responses so the handler-level cleanup is reliable.
…Service.cs Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
src/Microsoft.Agents.A365.DevTools.Cli/Commands/CleanupCommand.cs
Outdated
Show resolved
Hide resolved
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
src/Microsoft.Agents.A365.DevTools.Cli/Commands/CleanupCommand.cs
Outdated
Show resolved
Hide resolved
src/Microsoft.Agents.A365.DevTools.Cli/Services/AgentBlueprintService.cs
Outdated
Show resolved
Hide resolved
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
|
@gwharris7 I've opened a new pull request, #305, to work on those changes. Once the pull request is ready, I'll request review from you. |
#305) * Initial plan * Surface query failure explicitly in GetAgentInstancesForBlueprintAsync Co-authored-by: gwharris7 <96964444+gwharris7@users.noreply.github.com> --------- Co-authored-by: copilot-swe-agent[bot] <198982749+Copilot@users.noreply.github.com> Co-authored-by: gwharris7 <96964444+gwharris7@users.noreply.github.com>
| logger.LogWarning(""); | ||
| Console.WriteLine(); | ||
| logger.LogWarning("Blueprint deletion failed."); | ||
| PrintOrphanSummary(logger, failedResources); |
There was a problem hiding this comment.
1ES PT Branch Validation blocks official NuGet publish for GitHub‑hosted pipeline.
Scenario: one instance existed, its SP and agentic user were deleted successfully, but
then the blueprint deletion itself failed. Result: failedResources is empty, so
PrintOrphanSummary returns early. The user sees only "Blueprint deletion failed." with
no further guidance. The instances are already gone (their SP objects were deleted), but
the blueprint still exists in Entra ID. This is a partial-success state requiring explicit
user action.
The test `CleanupBlueprint_WhenBlueprintDeletionFailsWithInstances_LogsWarning` (diff line
~739) exercises this exact path and only asserts that "Blueprint deletion failed." was
logged, missing the guidance gap.
Suggested fix: emit actionable guidance whenever blueprint deletion fails, regardless of
orphaned resources:
if (!deleted)
{
logger.LogWarning("");
logger.LogWarning("Blueprint deletion failed. The blueprint still exists in Entra ID.");
PrintOrphanSummary(logger, failedResources);
if (!HasOrphanedResources(failedResources))
{
logger.LogWarning("All agent instances were deleted. Retry 'a365 cleanup blueprint' " +
"or delete the blueprint manually via the Entra portal or Graph API.");
}
return;
} Scenario: one instance existed, its SP and agentic user were deleted successfully, but
then the blueprint deletion itself failed. Result: `failedResources` is empty, so
`PrintOrphanSummary` returns early. The user sees only "Blueprint deletion failed." with
no further guidance. The instances are already gone (their SP objects were deleted), but
the blueprint still exists in Entra ID. This is a partial-success state requiring explicit
user action.
Suggested fix: emit actionable guidance whenever blueprint deletion fails, regardless of
orphaned resources:
if (!deleted)
{
logger.LogWarning("");
logger.LogWarning("Blueprint deletion failed. The blueprint still exists in Entra ID.");
PrintOrphanSummary(logger, failedResources);
if (!HasOrphanedResources(failedResources))
{
logger.LogWarning("All agent instances were deleted. Retry 'a365 cleanup blueprint' " +
"or delete the blueprint manually via the Entra portal or Graph API.");
}
return;
}
Update `CleanupBlueprint_WhenBlueprintDeletionFailsWithInstances_LogsWarning` to assert
that the retry guidance message is also emitted.
| if (!deleted) | ||
| { | ||
| logger.LogWarning(""); | ||
| Console.WriteLine(); |
There was a problem hiding this comment.
Console.WriteLine() mixed with ILogger in new production code -- inconsistent output channel
This pull request introduces significant improvements to the blueprint cleanup process in the CLI tool, focusing on cascading deletion of agent instances and better handling/reporting of partial failures. The changes enhance user experience by previewing all resources that will be deleted, confirming destructive actions, and providing clear warnings and summaries if any resources could not be deleted. Additionally, several new methods were added to support querying and deleting agent instances and users, and the test code was updated for compatibility. This PR addresses issue #258.
Blueprint cleanup enhancements:
The blueprint cleanup command now queries for all agent instances linked to a blueprint, shows a detailed preview of what will be deleted (including agentic users and identity service principals), and deletes these resources before deleting the blueprint itself. If any deletions fail, the user receives a summary of orphaned resources to clean up manually. [1] [2] [3] [4] [5]
The command now uses an injected
IConfirmationProviderfor user confirmation, replacing directConsole.ReadLinecalls, making the code more testable and consistent. [1] [2]Agent instance management:
Added a new
AgentInstanceInforecord inModels/AgentInstanceInfo.csto represent agent instances linked to a blueprint, including identity SP and optional agentic user.Introduced new methods in
AgentBlueprintServiceto:GetAgentInstancesForBlueprintAsync)DeleteAgentUserAsync)DeleteAgentBlueprintAsyncandDeleteAgentIdentityAsyncvirtual for easier testing/mocking. [1] [2] [3]Test updates: