feat: add azd tool uninstall command with per-host skill support#8794
Open
hemarina wants to merge 11 commits into
Open
feat: add azd tool uninstall command with per-host skill support#8794hemarina wants to merge 11 commits into
azd tool uninstall command with per-host skill support#8794hemarina wants to merge 11 commits into
Conversation
…rina/azure-dev into hemarina/azd-tool-uninstall
…na/azd-tool-uninstall
…rina/azure-dev into hemarina/azd-tool-uninstall
…rina/azure-dev into hemarina/azd-tool-uninstall
Contributor
There was a problem hiding this comment.
Pull request overview
Adds the missing azd tool uninstall subcommand to complete the managed tool lifecycle, including special handling for skill tools that are installed via agent-host CLIs (per-host uninstall support) and corresponding telemetry/docs updates.
Changes:
- Introduces
azd tool uninstallwith--all,--dry-run, and skill-only--hostsupport (including JSON output). - Extends the tool manifest and installer/manager layers to support uninstall across package managers, explicit uninstall commands, direct-download artifacts, and per-host skill removal + verification.
- Updates telemetry coverage tests, telemetry reference docs, the metrics-audit matrix, and regenerated usage/Fig spec snapshots.
Reviewed changes
Copilot reviewed 13 out of 13 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| docs/specs/metrics-audit/feature-telemetry-matrix.md | Adds tool uninstall to the command→telemetry inventory/matrix. |
| docs/reference/telemetry-data.md | Updates tool-management telemetry field descriptions to include uninstall semantics. |
| cli/azd/pkg/tool/manifest.go | Adds uninstall metadata (PluginUninstallCommand, UninstallCommand) to the tool manifest model + populates for skills and azd extensions. |
| cli/azd/pkg/tool/manager.go | Adds UninstallTools orchestration, including skill-first ordering for --all / batch scenarios. |
| cli/azd/pkg/tool/manager_test.go | Unit tests for UninstallTools delegation, option forwarding, batching behavior, and ordering. |
| cli/azd/pkg/tool/installer.go | Implements uninstall logic for skills + non-skills, adds uninstall command builders, and direct-download artifact removal. |
| cli/azd/pkg/tool/installer_test.go | Extensive installer uninstall test coverage across strategies and skill hosts. |
| cli/azd/cmd/tool.go | Wires up the tool uninstall Cobra/action implementation (flags, host resolution, dry-run, telemetry). |
| cli/azd/cmd/tool_test.go | Command-level tests for uninstall host resolution and telemetry/dry-run behavior. |
| cli/azd/cmd/testdata/TestUsage-azd-tool.snap | Updates azd tool usage snapshot to list uninstall. |
| cli/azd/cmd/testdata/TestUsage-azd-tool-uninstall.snap | New usage snapshot for azd tool uninstall. |
| cli/azd/cmd/testdata/TestFigSpec.ts | Adds Fig completion spec for azd tool uninstall. |
| cli/azd/cmd/telemetry_coverage_test.go | Registers tool uninstall under commands with specific telemetry. |
jongio
requested changes
Jun 25, 2026
jongio
left a comment
jongio
left a comment
Member
There was a problem hiding this comment.
The production code is solid and follows existing patterns well. One test has a mock setup bug that causes CI failures on all platforms.
TestUninstall_NonSkill_CommandFails uses &mockDetector{} which defaults to Installed: false. After the npm command fails, runUninstall checks whether the tool is still detected. Since the mock reports it as not installed, the idempotent success path fires (result.Success = true) instead of the intended error path. The test then fails its assert.False(t, result.Success) assertion.
Fix: use installedDetector("1.0.0") so the tool is still detected after the failed command, triggering the packageManagerUninstallFailedError path the test intends to exercise.
…na/azd-tool-uninstall
Collaborator
Azure Dev CLI Install InstructionsInstall scriptsMacOS/Linux
bash: pwsh: WindowsPowerShell install MSI install Standalone Binary
MSI
Documentationlearn.microsoft.com documentationtitle: Azure Developer CLI reference
|
jongio
reviewed
Jun 26, 2026
jongio
left a comment
jongio
left a comment
Member
There was a problem hiding this comment.
Prior feedback addressed: TestUninstall_NonSkill_CommandFails now uses installedDetector("1.0.0"), correctly exercising the failure path.
The skill-before-host uninstall ordering in UninstallTools is a solid design choice with proper test coverage. One minor observation below on dry-run output for skills.
| return nil, fmt.Errorf("detecting %s: %w", t.Id, detectErr) | ||
| } | ||
|
|
||
| action := "uninstall" |
Member
There was a problem hiding this comment.
For skill tools, the dry-run preview shows uninstall or skip (not installed) without indicating which host(s) the skill would be removed from. Since --host targeting is a key feature of skill uninstall, consider showing the target hosts here (e.g., uninstall from copilot, claude). Not blocking; just a discoverability improvement for users previewing skill removal.
jongio
reviewed
Jun 26, 2026
jongio
left a comment
jongio
left a comment
Member
There was a problem hiding this comment.
Prior feedback addressed: TestUninstall_NonSkill_CommandFails now uses installedDetector("1.0.0"), correctly exercising the failure path.
The skill-before-host uninstall ordering in UninstallTools is a solid design choice with proper test coverage. One minor observation below on dry-run output for skills.
| return nil, fmt.Errorf("detecting %s: %w", t.Id, detectErr) | ||
| } | ||
|
|
||
| action := "uninstall" |
Member
There was a problem hiding this comment.
For skill tools, the dry-run preview shows uninstall or skip (not installed) without indicating which host(s) the skill would be removed from. Since --host targeting is a key feature of skill uninstall, consider showing the target hosts here (e.g., uninstall from copilot, claude). Not blocking; just a discoverability improvement for users previewing skill removal.
RickWinter
reviewed
Jun 26, 2026
RickWinter
left a comment
RickWinter
left a comment
Member
There was a problem hiding this comment.
This adds azd tool uninstall as the inverse of tool install, threading a new Uninstall through the Installer interface, Manager.UninstallTools, and the cmd action, plus --all, --dry-run, and --host for skills. The shape is right: it mirrors the install/upgrade action structure, reuses the same telemetry emitter, keeps the tool.id vs tool.ids mutual-exclusion discipline, and the idempotent "already gone -> success" handling on every removal path is the correct call. Skill-before-host ordering in UninstallTools (so the host CLI is still on PATH to remove the plugin) is a genuinely good catch and is well covered by tests.
The one thing worth resolving before merge is the duplicated explicit-host resolution loop in installer.go: resolveSkillUninstallTargets copies the existing install-path block verbatim, which is exactly the cross-scope duplication AGENTS.md asks us to hoist. Not blocking, but cheap to fix now while both are fresh.
Nothing here is a security or correctness blocker. Coverage is thorough (manager, installer, and cmd layers all exercised), the docs/telemetry matrices are updated, and the snapshot regeneration is included. Disposition: COMMENT. Address the duplication if you agree; everything else is follow-up at most.
| idx := slices.IndexFunc(tool.SkillHosts, func(h SkillHost) bool { | ||
| return h.Host == name | ||
| }) | ||
| if idx < 0 || i.commandRunner.ToolInPath(name) != nil { |
Member
There was a problem hiding this comment.
This explicit-host block (the IndexFunc lookup, the idx < 0 || ToolInPath(name) != nil guard, and the supported list built for the error) is byte-for-byte identical to the install/upgrade path further down in this file (around line 1062). AGENTS.md specifically calls out not copying logic between scopes and extracting a shared helper instead. Can we pull this into something like resolveExplicitSkillHosts(tool, hosts) ([]SkillHost, error) and call it from both? Otherwise a future change to the host-availability rule (say, allowing a host that is configured but not yet on PATH) has to be made in two places, and they will drift.
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.
Fixes #8716
Summary
Adds the missing
azd tool uninstallcommand to theazd toolgroup, completing the install / upgrade / uninstall lifecycle. The command removes installed tools using the same mechanism each tool was installed through, and adds per-host support for skills.Behavior
azd tool uninstall <tool...>— uninstall the named tool(s).--all— uninstall every installed tool.--dry-run— preview what would be uninstalled without making changes.--output json— structured per-tool results.Per-host skills (
--host)For skill tools (e.g.
azure-skills, which install through agent CLIs like copilot/claude):azd tool uninstall <skill>— removes the skill from every host it is installed through.azd tool uninstall <skill> --host copilot— removes it from that host only.azd tool uninstall <skill> --host all— removes it from every detected host.--hosterrors on non-skill tools.Uninstall coverage by tool category
winget uninstall/brew uninstall/apt-get remove/npm uninstall -gcode --uninstall-extension <id>npm uninstall -gplugin uninstall(copilot, claude)azd extension uninstall <id>(via newInstallStrategy.UninstallCommand)After running the uninstall command, success is verified by re-detecting that the tool is no longer present (inverted from install, with the same retry/backoff for non-CLI tools).
Implementation
pkg/tool/installer.go—Uninstallon theInstallerinterface;runUninstall(package-manager / explicitUninstallCommand/ direct-download) andrunSkillUninstall(per-host removal + host-scoped verification);buildUninstallCommand,executeUninstall,executeUninstallCommand,executeDirectDownloadUninstall, and dedicated error builders.pkg/tool/manifest.go— newSkillHost.PluginUninstallCommand(populated for copilot/claude) andInstallStrategy.UninstallCommand(populated for the azd extension).pkg/tool/manager.go—UninstallTools(ctx, ids, opts...), forwarding host options; per-tool failures don't abort the batch; dependencies are intentionally left in place.cmd/tool.go— theuninstallsubcommand,toolUninstallAction/flags,--hostresolution (shared with install/upgrade), and dry-run, reusing the existingrunToolOperationand telemetry plumbing.Telemetry
Reuses the existing
tool.id/tool.ids,tool.dry_run, andtool.install.*aggregate fields (no new fields/events). Updatedtelemetry-coverage_test.go,docs/reference/telemetry-data.md, anddocs/specs/metrics-audit/feature-telemetry-matrix.md.Tests
UninstallCommandpath, still-detected failure, unsupported strategy,buildUninstallCommandtable, and skill per-host removal (default / explicit host / not-installed / unknown host).UninstallToolsdelegation, option forwarding, partial-failure batch, unknown id.TestUsage/TestFigSpecsnapshots.Notes
azd tool uninstallis unavailable for tools with only a custom shellInstallCommandand no reverse command / package manager; those return a clear "remove manually" message.sudo(apt) apply equally to install and uninstall.