From 3b163b10d55488160011ef4c1dd4d999ded478a1 Mon Sep 17 00:00:00 2001 From: hemarina Date: Wed, 24 Jun 2026 13:50:28 -0700 Subject: [PATCH 1/5] fix bug caused by copilot on path (from vscode included extension) identified as copilot cli --- cli/azd/pkg/tool/installer.go | 71 ++++++++++++++++++----- cli/azd/pkg/tool/installer_test.go | 93 +++++++++++++++++++++++++++--- cli/azd/pkg/tool/manifest.go | 25 ++++++++ 3 files changed, 167 insertions(+), 22 deletions(-) diff --git a/cli/azd/pkg/tool/installer.go b/cli/azd/pkg/tool/installer.go index f6b6b155d83..afcdbcca6da 100644 --- a/cli/azd/pkg/tool/installer.go +++ b/cli/azd/pkg/tool/installer.go @@ -561,13 +561,14 @@ func (i *installer) resolveSkillTargets( if allAvailable { var onPath []SkillHost for _, host := range tool.SkillHosts { - if i.commandRunner.ToolInPath(host.Host) == nil { + if i.hostUsable(ctx, host) { onPath = append(onPath, host) } } - // No host CLI present at all — surface the install guidance. + // No usable host CLI present at all — surface the install + // guidance. if len(onPath) == 0 { - host, err := i.pickSkillHost(tool) + host, err := i.pickSkillHost(ctx, tool) if err != nil { return nil, err } @@ -660,7 +661,7 @@ func (i *installer) resolveSkillTargets( ), } } - host, err := i.pickSkillHost(tool) + host, err := i.pickSkillHost(ctx, tool) if err != nil { return nil, err } @@ -670,13 +671,13 @@ func (i *installer) resolveSkillTargets( targets := make([]SkillHost, 0, len(hosts)) for _, name := range hosts { // A requested host is usable only if it is a configured SkillHost - // that is also on PATH. "unknown name" and "known but not on PATH" - // both mean the host can't be used, so we point the user at the - // supported hosts. + // that is a functional CLI on PATH. "unknown name", "not on PATH" + // and "present but a launcher stub" all mean the host can't be + // used, so we point the user at the supported hosts. idx := slices.IndexFunc(tool.SkillHosts, func(h SkillHost) bool { return h.Host == name }) - if idx < 0 || i.commandRunner.ToolInPath(name) != nil { + if idx < 0 || !i.hostUsable(ctx, tool.SkillHosts[idx]) { supported := make([]string, len(tool.SkillHosts)) for j, h := range tool.SkillHosts { supported[j] = h.Host @@ -691,18 +692,58 @@ func (i *installer) resolveSkillTargets( return targets, nil } -// pickSkillHost returns the first SkillHost whose binary is on PATH. -// When none of the configured hosts is available it returns an -// [errorhandler.ErrorWithSuggestion] (all four fields populated per the -// AGENTS.md completeness rule) that recommends installing GitHub -// Copilot CLI via `azd tool install github-copilot-cli` — a single -// command the user can copy-paste without leaving azd. +// hostUsable reports whether an agentic CLI host is a *functional* +// installation rather than merely a file named like the host on PATH. +// +// Some environments place a launcher stub on PATH that is not the +// real CLI — most notably the VS Code GitHub Copilot Chat extension, which +// drops a small `copilot` stub into its globalStorage and adds that folder +// to the integrated terminal's PATH. Invoking the stub only prints +// "Install GitHub Copilot CLI?" and exits 0 without doing anything, so it +// satisfies a bare exec.LookPath existence check yet cannot install the +// skill — which previously surfaced as the misleading +// " was installed via but verification failed". +// +// hostUsable runs the host's configured version probe and treats the host +// as usable only when it reports a real version. The probe runs +// non-interactively with an empty stdin, so a stub that prompts for input +// reads EOF and exits instead of hanging. Hosts that do not configure a +// version probe (BinaryVersionArgs/BinaryVersionRegex) fall back to the +// existence check. +func (i *installer) hostUsable(ctx context.Context, host SkillHost) bool { + if i.commandRunner.ToolInPath(host.Host) != nil { + return false + } + if len(host.BinaryVersionArgs) == 0 || host.BinaryVersionRegex == "" { + return true + } + result, err := i.commandRunner.Run( + ctx, + exec.NewRunArgs(host.Host, host.BinaryVersionArgs...). + WithStdIn(strings.NewReader("")), + ) + // A cancelled/timed-out probe is not evidence the host is a stub; do + // not penalize it here (context handling is the caller's concern). + if isContextErr(err) { + return true + } + return matchVersion(result.Stdout+result.Stderr, host.BinaryVersionRegex) != "" +} + +// pickSkillHost returns the first SkillHost that is a usable (functional) +// CLI — see [installer.hostUsable], which rejects launcher stubs that merely +// share the host's name on PATH. When none of the configured hosts is usable +// it returns an [errorhandler.ErrorWithSuggestion] (all four fields populated +// per the AGENTS.md completeness rule) that recommends installing GitHub +// Copilot CLI via `azd tool install github-copilot-cli` — a single command +// the user can copy-paste without leaving azd. func (i *installer) pickSkillHost( + ctx context.Context, tool *ToolDefinition, ) (SkillHost, error) { var checked []string for _, host := range tool.SkillHosts { - if err := i.commandRunner.ToolInPath(host.Host); err == nil { + if i.hostUsable(ctx, host) { return host, nil } checked = append(checked, host.Host) diff --git a/cli/azd/pkg/tool/installer_test.go b/cli/azd/pkg/tool/installer_test.go index 83615172ea4..a5532a1ef2f 100644 --- a/cli/azd/pkg/tool/installer_test.go +++ b/cli/azd/pkg/tool/installer_test.go @@ -1149,6 +1149,8 @@ func newSkillTool() *ToolDefinition { PluginUpdateCommand: []string{"plugin", "update", "azure@azure-skills"}, PluginListCommand: []string{"plugin", "list"}, PluginName: "azure@azure-skills", + BinaryVersionArgs: []string{"--version"}, + BinaryVersionRegex: `(\d+\.\d+\.\d+)`, }, { Host: "claude", @@ -1157,6 +1159,8 @@ func newSkillTool() *ToolDefinition { PluginUpdateCommand: []string{"plugin", "update", "azure@azure-skills"}, PluginListCommand: []string{"plugin", "list", "azure@azure-skills"}, PluginName: "azure@azure-skills", + BinaryVersionArgs: []string{"--version"}, + BinaryVersionRegex: `(\d+\.\d+\.\d+)`, }, }, } @@ -1164,7 +1168,10 @@ func newSkillTool() *ToolDefinition { // mockHostPresence wires ToolInPath responses so only the named hosts // resolve successfully. Pass an empty slice to mock every host as -// missing. +// missing. Present hosts also get a version-probe response (a real semver) +// so installer.hostUsable treats them as genuine, functional CLIs rather +// than launcher stubs. Tests that want to simulate a stub register their +// own later "--version" expectation, which wins (last match). func mockHostPresence( runner *mockexec.MockCommandRunner, present ...string, @@ -1172,6 +1179,10 @@ func mockHostPresence( for _, h := range allSkillHostNames { if slices.Contains(present, h) { runner.MockToolInPath(h, nil) + host := h + runner.When(func(args exec.RunArgs, _ string) bool { + return args.Cmd == host && slices.Contains(args.Args, "--version") + }).Respond(exec.RunResult{Stdout: "1.1.70", ExitCode: 0}) } else { runner.MockToolInPath(h, errors.New("not found")) } @@ -1368,6 +1379,68 @@ func TestRunSkill_NoHost_FailsWithSuggestion(t *testing.T) { assert.Contains(t, ews.Links[0].Title, "GitHub Copilot CLI") } +// TestRunSkill_Install_LauncherStubHost_ReturnsInstallGuidance verifies +// that a host binary present on PATH but non-functional — e.g. the VS Code +// Copilot Chat extension's `copilot` launcher stub, which only prompts to +// install the real CLI and exits 0 — is not mistaken for a usable host. +// The install must fail with the same clean "install GitHub Copilot CLI" +// guidance as a host that is entirely absent (matching Windows), never the +// misleading "was installed via copilot but verification failed", and must +// not attempt any plugin command through the stub. +func TestRunSkill_Install_LauncherStubHost_ReturnsInstallGuidance(t *testing.T) { + t.Parallel() + + runner := mockexec.NewMockCommandRunner() + // copilot is on PATH (the launcher stub); claude is not. + runner.MockToolInPath("copilot", nil) + runner.MockToolInPath("claude", errors.New("not found")) + runner.MockToolInPath("node", nil) + + // The launcher stub answers `copilot --version` with its install + // prompt instead of a version, and exits 0. + runner.When(func(args exec.RunArgs, _ string) bool { + return args.Cmd == "copilot" && slices.Contains(args.Args, "--version") + }).Respond(exec.RunResult{ + Stdout: "Install GitHub Copilot CLI? ['y/N']", + Stderr: "Cannot find GitHub Copilot CLI (https://docs.github.com/copilot)", + ExitCode: 0, + }) + + // Fail the assertion if any plugin command is attempted via the stub. + var attemptedInstall bool + runner.When(func(args exec.RunArgs, _ string) bool { + return args.Cmd == "copilot" && + (slices.Contains(args.Args, "install") || + slices.Contains(args.Args, "marketplace") || + slices.Contains(args.Args, "list")) + }).RespondFn(func(_ exec.RunArgs) (exec.RunResult, error) { + attemptedInstall = true + return exec.RunResult{ExitCode: 0}, nil + }) + + inst := NewInstaller( + runner, NewPlatformDetector(runner), installedDetector("1.1.70"), + ) + + result, err := inst.Install(t.Context(), newSkillTool()) + require.NoError(t, err) + require.False(t, result.Success) + require.NotNil(t, result.Error) + + // Clean, actionable guidance — not "verification failed". + ews, ok := errors.AsType[*errorhandler.ErrorWithSuggestion](result.Error) + require.True(t, ok, + "expected *errorhandler.ErrorWithSuggestion, got %T: %v", + result.Error, result.Error, + ) + assert.Contains(t, ews.Suggestion, "azd tool install github-copilot-cli") + assert.NotContains(t, result.Error.Error(), "verification failed") + + // The stub must never be driven through an install/marketplace flow. + assert.False(t, attemptedInstall, + "must not attempt to install through a non-functional launcher stub") +} + // --------------------------------------------------------------------------- // runSkill — node soft prereq // --------------------------------------------------------------------------- @@ -1383,7 +1456,8 @@ func TestRunSkill_NodeMissing_WarnsButProceeds(t *testing.T) { runner.MockToolInPath("node", errors.New("not found")) runner.When(func(args exec.RunArgs, _ string) bool { - return args.Cmd == "copilot" + return args.Cmd == "copilot" && + !slices.Contains(args.Args, "--version") }).Respond(exec.RunResult{ExitCode: 0}) inst := NewInstaller( @@ -1857,7 +1931,8 @@ func TestRunSkill_MarketplaceRealError_FailsBeforeInstall(t *testing.T) { runner.When(func(args exec.RunArgs, _ string) bool { return args.Cmd == "copilot" && - !slices.Contains(args.Args, "marketplace") + !slices.Contains(args.Args, "marketplace") && + !slices.Contains(args.Args, "--version") }).RespondFn(func(_ exec.RunArgs) (exec.RunResult, error) { installRan = true return exec.RunResult{ExitCode: 0}, nil @@ -1896,7 +1971,8 @@ func TestRunSkill_InstallCommandFails_SurfacesError(t *testing.T) { wantErr := errors.New("exit status 2") runner.When(func(args exec.RunArgs, _ string) bool { - return args.Cmd == "claude" + return args.Cmd == "claude" && + !slices.Contains(args.Args, "--version") }).RespondFn(func(_ exec.RunArgs) (exec.RunResult, error) { return exec.RunResult{ExitCode: 2, Stderr: "Network unreachable"}, wantErr }) @@ -1955,7 +2031,8 @@ func TestRunSkill_VerifyDetectorError_Surfaces(t *testing.T) { mockHostPresence(runner, "copilot") runner.MockToolInPath("node", nil) runner.When(func(args exec.RunArgs, _ string) bool { - return args.Cmd == "copilot" + return args.Cmd == "copilot" && + !slices.Contains(args.Args, "--version") }).Respond(exec.RunResult{ExitCode: 0}) wantErr := errors.New("plugin list failed") @@ -1988,7 +2065,8 @@ func TestRunSkill_VerificationFails_AfterRetries(t *testing.T) { mockHostPresence(runner, "copilot") runner.MockToolInPath("node", nil) runner.When(func(args exec.RunArgs, _ string) bool { - return args.Cmd == "copilot" + return args.Cmd == "copilot" && + !slices.Contains(args.Args, "--version") }).Respond(exec.RunResult{ExitCode: 0}) // Detection never reports the skill installed, so verification @@ -2027,7 +2105,8 @@ func TestRunSkill_ContextCanceledDuringVerify(t *testing.T) { mockHostPresence(runner, "copilot") runner.MockToolInPath("node", nil) runner.When(func(args exec.RunArgs, _ string) bool { - return args.Cmd == "copilot" + return args.Cmd == "copilot" && + !slices.Contains(args.Args, "--version") }).Respond(exec.RunResult{ExitCode: 0}) var callCount atomic.Int32 diff --git a/cli/azd/pkg/tool/manifest.go b/cli/azd/pkg/tool/manifest.go index 458e11a0ea6..c2bbb553af1 100644 --- a/cli/azd/pkg/tool/manifest.go +++ b/cli/azd/pkg/tool/manifest.go @@ -80,6 +80,23 @@ type SkillHost struct { // captured group as InstalledVersion). A host with an empty // VersionRegex is never reported as installed. VersionRegex string + // BinaryVersionArgs are the CLI arguments that make the host binary + // print its own version (e.g. ["--version"]). Together with + // BinaryVersionRegex these let the installer confirm the host is a + // genuine, functional CLI before installing through it — not merely a + // file of the same name on PATH. Some environments place a launcher + // stub on PATH (e.g. the VS Code GitHub Copilot Chat extension drops a + // small `copilot` stub into its globalStorage and adds that folder to + // the integrated terminal's PATH) that exits 0 but only prompts to + // install the real CLI; such a stub passes a bare PATH existence check + // yet cannot install the skill. When empty, the installer falls back to + // an existence-only check. + BinaryVersionArgs []string + // BinaryVersionRegex is a Go regular expression whose first capture + // group matches the host binary's own version (e.g. `\d+\.\d+\.\d+`). + // The installer treats a match as proof the host CLI is genuinely + // installed. When empty, the functional probe is skipped. + BinaryVersionRegex string } // InstallStrategy describes how to install a tool on a specific platform. @@ -353,6 +370,10 @@ func azureSkills() *ToolDefinition { PluginName: "azure@azure-skills", // Sample: " • azure@azure-skills (v1.1.70)" VersionRegex: `azure@azure-skills[^\n]*?(\d+\.\d+\.\d+)`, + // Probe the host binary itself so a launcher stub that only + // prompts to install the real CLI is not mistaken for a host. + BinaryVersionArgs: []string{"--version"}, + BinaryVersionRegex: `(\d+\.\d+\.\d+)`, }, { Host: "claude", @@ -368,6 +389,10 @@ func azureSkills() *ToolDefinition { // Claude only returns the queried plugin, so a single // "Version: x.y.z" line is unambiguous. VersionRegex: `Version:\s*v?(\d+\.\d+\.\d+)`, + // Probe the host binary itself so a launcher stub that only + // prompts to install the real CLI is not mistaken for a host. + BinaryVersionArgs: []string{"--version"}, + BinaryVersionRegex: `(\d+\.\d+\.\d+)`, }, }, } From d5f428f6576bad2e38b7a0557a1d1b7487240c32 Mon Sep 17 00:00:00 2001 From: hemarina Date: Wed, 24 Jun 2026 17:37:19 -0700 Subject: [PATCH 2/5] address comment, close gap --- cli/azd/cmd/tool.go | 4 ++-- cli/azd/cmd/tool_test.go | 10 +++++----- cli/azd/pkg/tool/installer.go | 22 +++++++++++++--------- cli/azd/pkg/tool/installer_test.go | 4 ++-- cli/azd/pkg/tool/manager.go | 10 +++++----- cli/azd/pkg/tool/manager_test.go | 10 +++++----- 6 files changed, 32 insertions(+), 28 deletions(-) diff --git a/cli/azd/cmd/tool.go b/cli/azd/cmd/tool.go index e9df6da6374..cdc21cfaf88 100644 --- a/cli/azd/cmd/tool.go +++ b/cli/azd/cmd/tool.go @@ -673,7 +673,7 @@ func (a *toolInstallAction) resolveHostOptions( // Explicitly-named skill: when multiple hosts are detected we cannot // safely guess which the user wants. - present := a.manager.AvailableSkillHosts(skill) + present := a.manager.AvailableSkillHosts(ctx, skill) if len(present) > 1 { // Interactive terminal: prompt the user to pick the host(s), // after surfacing the --host hint so they learn the shortcut too. @@ -730,7 +730,7 @@ func (a *toolInstallAction) resolveUnavailableHostPrompt( return nil, false, nil } - available := a.manager.AvailableSkillHosts(skill) + available := a.manager.AvailableSkillHosts(ctx, skill) var unavailable []string for _, host := range a.flags.hosts { if !slices.Contains(available, host) { diff --git a/cli/azd/cmd/tool_test.go b/cli/azd/cmd/tool_test.go index 66d85150447..6bded05e828 100644 --- a/cli/azd/cmd/tool_test.go +++ b/cli/azd/cmd/tool_test.go @@ -347,7 +347,7 @@ type cmdMockInstaller struct { upgrade func( ctx context.Context, t *tool.ToolDefinition, opts ...tool.InstallOption, ) (*tool.InstallResult, error) - availableSkillHosts func(t *tool.ToolDefinition) []string + availableSkillHosts func(ctx context.Context, t *tool.ToolDefinition) []string } func (m *cmdMockInstaller) Install( @@ -368,9 +368,9 @@ func (m *cmdMockInstaller) Upgrade( return &tool.InstallResult{Tool: t, Success: true}, nil } -func (m *cmdMockInstaller) AvailableSkillHosts(t *tool.ToolDefinition) []string { +func (m *cmdMockInstaller) AvailableSkillHosts(ctx context.Context, t *tool.ToolDefinition) []string { if m.availableSkillHosts != nil { - return m.availableSkillHosts(t) + return m.availableSkillHosts(ctx, t) } return nil } @@ -651,7 +651,7 @@ func TestResolveHostOptions(t *testing.T) { newAction := func(args []string, flags *toolInstallFlags, present []string) *toolInstallAction { installer := &cmdMockInstaller{ - availableSkillHosts: func(_ *tool.ToolDefinition) []string { + availableSkillHosts: func(_ context.Context, _ *tool.ToolDefinition) []string { return present }, } @@ -932,7 +932,7 @@ func TestResolveHostOptions_Upgrade(t *testing.T) { newAction := func(flags *toolUpgradeFlags, present []string) *toolUpgradeAction { installer := &cmdMockInstaller{ - availableSkillHosts: func(_ *tool.ToolDefinition) []string { + availableSkillHosts: func(_ context.Context, _ *tool.ToolDefinition) []string { return present }, } diff --git a/cli/azd/pkg/tool/installer.go b/cli/azd/pkg/tool/installer.go index afcdbcca6da..9ddf34419ec 100644 --- a/cli/azd/pkg/tool/installer.go +++ b/cli/azd/pkg/tool/installer.go @@ -112,10 +112,11 @@ type Installer interface { ) (*InstallResult, error) // AvailableSkillHosts returns the names of the tool's configured - // SkillHosts that are currently on PATH, in manifest order - // (preferred host first). It returns nil for non-skill tools or - // when none of the hosts are available. - AvailableSkillHosts(tool *ToolDefinition) []string + // SkillHosts that are currently usable (a functional CLI on PATH, per + // hostUsable), in manifest order (preferred host first). It returns nil + // for non-skill tools or when none of the hosts are usable. It probes + // the host binaries, so it takes a context. + AvailableSkillHosts(ctx context.Context, tool *ToolDefinition) []string } // installConfig holds the resolved options for an install or upgrade @@ -229,16 +230,19 @@ func (i *installer) Upgrade( } // AvailableSkillHosts returns the names of the tool's configured -// SkillHosts that are currently on PATH, in manifest order (preferred -// host first). It returns nil for non-skill tools or when none of the -// hosts are available. -func (i *installer) AvailableSkillHosts(tool *ToolDefinition) []string { +// SkillHosts that are currently usable, in manifest order (preferred host +// first). A host counts only if [installer.hostUsable] confirms it is a +// functional CLI — not merely a same-named launcher stub on PATH — so the +// interactive host picker never offers a host the install path would later +// reject. It returns nil for non-skill tools or when none of the hosts are +// usable. +func (i *installer) AvailableSkillHosts(ctx context.Context, tool *ToolDefinition) []string { if tool.Category != ToolCategorySkill { return nil } var present []string for _, host := range tool.SkillHosts { - if i.commandRunner.ToolInPath(host.Host) == nil { + if i.hostUsable(ctx, host) { present = append(present, host.Host) } } diff --git a/cli/azd/pkg/tool/installer_test.go b/cli/azd/pkg/tool/installer_test.go index a5532a1ef2f..d86a0dbf1dd 100644 --- a/cli/azd/pkg/tool/installer_test.go +++ b/cli/azd/pkg/tool/installer_test.go @@ -2287,7 +2287,7 @@ func TestAvailableSkillHosts_ReturnsPresentInManifestOrder(t *testing.T) { assert.Equal(t, []string{"copilot", "claude"}, - inst.AvailableSkillHosts(newSkillTool()), + inst.AvailableSkillHosts(t.Context(), newSkillTool()), ) } @@ -2297,7 +2297,7 @@ func TestAvailableSkillHosts_NonSkillToolReturnsNil(t *testing.T) { runner := mockexec.NewMockCommandRunner() inst := NewInstaller(runner, NewPlatformDetector(runner), &mockDetector{}) - assert.Nil(t, inst.AvailableSkillHosts(&ToolDefinition{ + assert.Nil(t, inst.AvailableSkillHosts(t.Context(), &ToolDefinition{ Id: "not-a-skill", Category: ToolCategoryServer, })) diff --git a/cli/azd/pkg/tool/manager.go b/cli/azd/pkg/tool/manager.go index f54514a1510..f1601489401 100644 --- a/cli/azd/pkg/tool/manager.go +++ b/cli/azd/pkg/tool/manager.go @@ -69,11 +69,11 @@ func (m *Manager) FindTool(id string) (*ToolDefinition, error) { } // AvailableSkillHosts returns the names of the given skill tool's -// configured agentic CLI hosts that are currently on PATH, in manifest -// order (preferred host first). It returns nil for non-skill tools or -// when none of the hosts are available. -func (m *Manager) AvailableSkillHosts(tool *ToolDefinition) []string { - return m.installer.AvailableSkillHosts(tool) +// configured agentic CLI hosts that are currently usable (a functional CLI +// on PATH), in manifest order (preferred host first). It returns nil for +// non-skill tools or when none of the hosts are usable. +func (m *Manager) AvailableSkillHosts(ctx context.Context, tool *ToolDefinition) []string { + return m.installer.AvailableSkillHosts(ctx, tool) } // DetectAll probes every tool in the manifest and returns a status diff --git a/cli/azd/pkg/tool/manager_test.go b/cli/azd/pkg/tool/manager_test.go index 9d15ca14ea7..65008557bb9 100644 --- a/cli/azd/pkg/tool/manager_test.go +++ b/cli/azd/pkg/tool/manager_test.go @@ -28,7 +28,7 @@ type mockInstaller struct { tool *ToolDefinition, opts ...InstallOption, ) (*InstallResult, error) - availableSkillHostsFn func(tool *ToolDefinition) []string + availableSkillHostsFn func(ctx context.Context, tool *ToolDefinition) []string } func (m *mockInstaller) Install( @@ -59,9 +59,9 @@ func (m *mockInstaller) Upgrade( }, nil } -func (m *mockInstaller) AvailableSkillHosts(tool *ToolDefinition) []string { +func (m *mockInstaller) AvailableSkillHosts(ctx context.Context, tool *ToolDefinition) []string { if m.availableSkillHostsFn != nil { - return m.availableSkillHostsFn(tool) + return m.availableSkillHostsFn(ctx, tool) } return nil } @@ -642,13 +642,13 @@ func TestManager_UpgradeAll(t *testing.T) { func TestManager_AvailableSkillHosts(t *testing.T) { installer := &mockInstaller{ - availableSkillHostsFn: func(_ *ToolDefinition) []string { + availableSkillHostsFn: func(_ context.Context, _ *ToolDefinition) []string { return []string{"copilot", "claude"} }, } m := NewManager(&mockDetector{}, installer, nil) - got := m.AvailableSkillHosts(&ToolDefinition{ + got := m.AvailableSkillHosts(t.Context(), &ToolDefinition{ Id: "azure-skills", Category: ToolCategorySkill, }) From c6e3cc8a9ae71ddc2438fd2b8aee960c5f465f7e Mon Sep 17 00:00:00 2001 From: hemarina Date: Thu, 25 Jun 2026 20:01:26 -0700 Subject: [PATCH 3/5] address feedback --- cli/azd/pkg/tool/installer.go | 55 ++++++++-- cli/azd/pkg/tool/installer_test.go | 160 ++++++++++++++++++++++++++--- cli/azd/pkg/tool/manifest.go | 19 +++- cli/azd/pkg/tool/manifest_test.go | 59 +++++++++++ 4 files changed, 269 insertions(+), 24 deletions(-) diff --git a/cli/azd/pkg/tool/installer.go b/cli/azd/pkg/tool/installer.go index 9ddf34419ec..46c3e03d3b3 100644 --- a/cli/azd/pkg/tool/installer.go +++ b/cli/azd/pkg/tool/installer.go @@ -162,6 +162,13 @@ type installer struct { httpClient httpDoer platformMu sync.Mutex platform *Platform // lazily populated by ensurePlatform + // hostProbe memoizes hostUsable's version-probe result per host for the + // installer's lifetime (one process == one command), so the several + // host-resolution call sites do not re-spawn `--version` for the same + // host. Only on-PATH results are stored (see hostUsable). Guarded by + // hostProbeMu. + hostProbeMu sync.Mutex + hostProbe map[string]bool // retryBackoff is the initial wait between post-install detection // retries (doubled each attempt). Defaults to 1s; tests may shorten // it to keep the suite fast. @@ -708,16 +715,50 @@ func (i *installer) resolveSkillTargets( // skill — which previously surfaced as the misleading // " was installed via but verification failed". // -// hostUsable runs the host's configured version probe and treats the host -// as usable only when it reports a real version. The probe runs -// non-interactively with an empty stdin, so a stub that prompts for input -// reads EOF and exits instead of hanging. Hosts that do not configure a -// version probe (BinaryVersionArgs/BinaryVersionRegex) fall back to the -// existence check. +// hostUsable shells out to the host's version command (non-interactively, +// with an empty stdin so a stub that prompts reads EOF and exits instead of +// hanging) and treats the host as usable only when the output matches the +// host's BinaryVersionRegex. That regex is anchored to the host's own +// `--version` banner (e.g. "^GitHub Copilot CLI "), so an incidental +// version-shaped token elsewhere in a stub's output does not count. Hosts +// that do not configure a version probe (BinaryVersionArgs/BinaryVersionRegex) +// fall back to the existence check. +// +// hostUsable is consulted from several host-resolution call sites within one +// command, so the version-probe result is memoized per host (hostProbe). Only +// on-PATH results are cached: a host that is not on PATH is re-checked every +// time (the exec.LookPath check is cheap) so a host installed earlier in the +// same batch — e.g. `azd tool install github-copilot-cli azure-skills` — is +// still picked up at run time. The cache assumes an on-PATH host binary is not +// swapped in place mid-command, which azd never does (it installs hosts to +// their own package locations). func (i *installer) hostUsable(ctx context.Context, host SkillHost) bool { if i.commandRunner.ToolInPath(host.Host) != nil { return false } + + i.hostProbeMu.Lock() + if i.hostProbe == nil { + i.hostProbe = map[string]bool{} + } + usable, ok := i.hostProbe[host.Host] + i.hostProbeMu.Unlock() + if ok { + return usable + } + + usable = i.probeOnPathHost(ctx, host) + + i.hostProbeMu.Lock() + i.hostProbe[host.Host] = usable + i.hostProbeMu.Unlock() + return usable +} + +// probeOnPathHost runs the version probe for a host already confirmed to be on +// PATH and reports whether it is a functional CLI. It is the uncached half of +// hostUsable; see that method for the rationale behind the matching. +func (i *installer) probeOnPathHost(ctx context.Context, host SkillHost) bool { if len(host.BinaryVersionArgs) == 0 || host.BinaryVersionRegex == "" { return true } @@ -731,7 +772,7 @@ func (i *installer) hostUsable(ctx context.Context, host SkillHost) bool { if isContextErr(err) { return true } - return matchVersion(result.Stdout+result.Stderr, host.BinaryVersionRegex) != "" + return matchVersion(result.Stdout+"\n"+result.Stderr, host.BinaryVersionRegex) != "" } // pickSkillHost returns the first SkillHost that is a usable (functional) diff --git a/cli/azd/pkg/tool/installer_test.go b/cli/azd/pkg/tool/installer_test.go index d86a0dbf1dd..8868e774bbc 100644 --- a/cli/azd/pkg/tool/installer_test.go +++ b/cli/azd/pkg/tool/installer_test.go @@ -1150,7 +1150,7 @@ func newSkillTool() *ToolDefinition { PluginListCommand: []string{"plugin", "list"}, PluginName: "azure@azure-skills", BinaryVersionArgs: []string{"--version"}, - BinaryVersionRegex: `(\d+\.\d+\.\d+)`, + BinaryVersionRegex: `(?m)^GitHub Copilot CLI\s+v?(\d+\.\d+\.\d+)`, }, { Host: "claude", @@ -1160,7 +1160,7 @@ func newSkillTool() *ToolDefinition { PluginListCommand: []string{"plugin", "list", "azure@azure-skills"}, PluginName: "azure@azure-skills", BinaryVersionArgs: []string{"--version"}, - BinaryVersionRegex: `(\d+\.\d+\.\d+)`, + BinaryVersionRegex: `(?m)^v?(\d+\.\d+\.\d+)\s+\(Claude Code\)`, }, }, } @@ -1168,21 +1168,29 @@ func newSkillTool() *ToolDefinition { // mockHostPresence wires ToolInPath responses so only the named hosts // resolve successfully. Pass an empty slice to mock every host as -// missing. Present hosts also get a version-probe response (a real semver) -// so installer.hostUsable treats them as genuine, functional CLIs rather -// than launcher stubs. Tests that want to simulate a stub register their -// own later "--version" expectation, which wins (last match). +// missing. Present hosts also get a version-probe response whose banner +// matches the host's anchored BinaryVersionRegex (see newSkillTool), so +// installer.hostUsable treats them as genuine, functional CLIs rather than +// launcher stubs. Tests that want to simulate a stub register their own +// later "--version" expectation, which wins (last match). func mockHostPresence( runner *mockexec.MockCommandRunner, present ...string, ) { + // Per-host `--version` banner that satisfies the host's anchored + // BinaryVersionRegex. + versionBanner := map[string]string{ + "copilot": "GitHub Copilot CLI 1.1.70", + "claude": "1.1.70 (Claude Code)", + } for _, h := range allSkillHostNames { if slices.Contains(present, h) { runner.MockToolInPath(h, nil) host := h + banner := versionBanner[h] runner.When(func(args exec.RunArgs, _ string) bool { return args.Cmd == host && slices.Contains(args.Args, "--version") - }).Respond(exec.RunResult{Stdout: "1.1.70", ExitCode: 0}) + }).Respond(exec.RunResult{Stdout: banner, ExitCode: 0}) } else { runner.MockToolInPath(h, errors.New("not found")) } @@ -1397,13 +1405,20 @@ func TestRunSkill_Install_LauncherStubHost_ReturnsInstallGuidance(t *testing.T) runner.MockToolInPath("node", nil) // The launcher stub answers `copilot --version` with its install - // prompt instead of a version, and exits 0. + // prompt instead of a version, and exits 0. Record that the probe + // actually ran and returned the stub's response, so this test fails + // loudly if mock precedence ever changed and the probe silently observed + // a different (e.g. version-shaped) response instead. + var stubVersionProbed bool runner.When(func(args exec.RunArgs, _ string) bool { return args.Cmd == "copilot" && slices.Contains(args.Args, "--version") - }).Respond(exec.RunResult{ - Stdout: "Install GitHub Copilot CLI? ['y/N']", - Stderr: "Cannot find GitHub Copilot CLI (https://docs.github.com/copilot)", - ExitCode: 0, + }).RespondFn(func(_ exec.RunArgs) (exec.RunResult, error) { + stubVersionProbed = true + return exec.RunResult{ + Stdout: "Install GitHub Copilot CLI? ['y/N']", + Stderr: "Cannot find GitHub Copilot CLI (https://docs.github.com/copilot)", + ExitCode: 0, + }, nil }) // Fail the assertion if any plugin command is attempted via the stub. @@ -1439,6 +1454,69 @@ func TestRunSkill_Install_LauncherStubHost_ReturnsInstallGuidance(t *testing.T) // The stub must never be driven through an install/marketplace flow. assert.False(t, attemptedInstall, "must not attempt to install through a non-functional launcher stub") + + // Self-defending: the probe must have actually executed and observed the + // stub's response (guards against a silent mock-precedence flip that would + // otherwise let a stub be treated as usable without failing this test). + assert.True(t, stubVersionProbed, + "hostUsable must probe `copilot --version` and observe the stub response") +} + +// TestRunSkill_Install_StubWithIncidentalVersion_Rejected verifies that a +// launcher stub is still rejected when its output contains a version-shaped +// token that is not a genuine version report — e.g. a bundled node version, a +// path build number, or even a line that starts with the real CLI's banner +// prefix but carries no version. BinaryVersionRegex is anchored to the host's +// `--version` banner, so only a real version line counts. +func TestRunSkill_Install_StubWithIncidentalVersion_Rejected(t *testing.T) { + t.Parallel() + + runner := mockexec.NewMockCommandRunner() + runner.MockToolInPath("copilot", nil) + runner.MockToolInPath("claude", errors.New("not found")) + runner.MockToolInPath("node", nil) + + // None of these lines is a genuine "GitHub Copilot CLI " + // report: one borrows the banner prefix without a version, others carry + // incidental semvers (a node runtime version, a URL build number). + runner.When(func(args exec.RunArgs, _ string) bool { + return args.Cmd == "copilot" && slices.Contains(args.Args, "--version") + }).Respond(exec.RunResult{ + Stdout: "GitHub Copilot CLI is not installed\n" + + "Install GitHub Copilot CLI? ['y/N']\n" + + "Downloading node v20.11.1 runtime\n" + + "See https://example.com/releases/1.2.3 for details", + ExitCode: 0, + }) + + // Fail loudly if any plugin command is attempted through the stub. + var attemptedInstall bool + runner.When(func(args exec.RunArgs, _ string) bool { + return args.Cmd == "copilot" && + (slices.Contains(args.Args, "install") || + slices.Contains(args.Args, "marketplace") || + slices.Contains(args.Args, "list")) + }).RespondFn(func(_ exec.RunArgs) (exec.RunResult, error) { + attemptedInstall = true + return exec.RunResult{ExitCode: 0}, nil + }) + + inst := NewInstaller( + runner, NewPlatformDetector(runner), installedDetector("1.1.70"), + ) + + result, err := inst.Install(t.Context(), newSkillTool()) + require.NoError(t, err) + require.False(t, result.Success, + "incidental version text must not make a stub usable") + require.NotNil(t, result.Error) + + _, ok := errors.AsType[*errorhandler.ErrorWithSuggestion](result.Error) + require.True(t, ok, + "expected install guidance, got %T: %v", result.Error, result.Error) + assert.NotContains(t, result.Error.Error(), "verification failed") + assert.False(t, attemptedInstall, + "must not install through a stub that only prints incidental version text") } // --------------------------------------------------------------------------- @@ -2302,3 +2380,61 @@ func TestAvailableSkillHosts_NonSkillToolReturnsNil(t *testing.T) { Category: ToolCategoryServer, })) } + +// TestHostUsable_OnPathProbeMemoizedAcrossPasses verifies that an on-PATH +// host's `--version` is spawned at most once per command even though host +// resolution probes it from several call sites: the result is memoized on the +// installer for its lifetime (one process == one command). +func TestHostUsable_OnPathProbeMemoizedAcrossPasses(t *testing.T) { + t.Parallel() + + runner := mockexec.NewMockCommandRunner() + runner.MockToolInPath("copilot", nil) + runner.MockToolInPath("claude", errors.New("not found")) + + var copilotVersionCalls int + runner.When(func(args exec.RunArgs, _ string) bool { + return args.Cmd == "copilot" && slices.Contains(args.Args, "--version") + }).RespondFn(func(_ exec.RunArgs) (exec.RunResult, error) { + copilotVersionCalls++ + return exec.RunResult{Stdout: "GitHub Copilot CLI 1.1.70", ExitCode: 0}, nil + }) + + inst := NewInstaller(runner, NewPlatformDetector(runner), &mockDetector{}) + + // Two separate host-resolution passes on the same installer. + require.Equal(t, []string{"copilot"}, + inst.AvailableSkillHosts(t.Context(), newSkillTool())) + require.Equal(t, []string{"copilot"}, + inst.AvailableSkillHosts(t.Context(), newSkillTool())) + + assert.Equal(t, 1, copilotVersionCalls, + "on-PATH host `--version` must be probed at most once per command") +} + +// TestHostUsable_NotOnPathResultNotMemoized verifies that a not-on-PATH result +// is never cached, so a host installed earlier in the same command (e.g. +// `azd tool install github-copilot-cli azure-skills`) is still picked up by a +// later resolution pass. +func TestHostUsable_NotOnPathResultNotMemoized(t *testing.T) { + t.Parallel() + + runner := mockexec.NewMockCommandRunner() + runner.MockToolInPath("copilot", errors.New("not found")) + runner.MockToolInPath("claude", errors.New("not found")) + + inst := NewInstaller(runner, NewPlatformDetector(runner), &mockDetector{}) + + // First pass: copilot absent. + require.Empty(t, inst.AvailableSkillHosts(t.Context(), newSkillTool())) + + // copilot becomes available and functional mid-command. + runner.MockToolInPath("copilot", nil) + runner.When(func(args exec.RunArgs, _ string) bool { + return args.Cmd == "copilot" && slices.Contains(args.Args, "--version") + }).Respond(exec.RunResult{Stdout: "GitHub Copilot CLI 1.1.70", ExitCode: 0}) + + // Second pass picks it up: the earlier "not on PATH" result was not cached. + assert.Equal(t, []string{"copilot"}, + inst.AvailableSkillHosts(t.Context(), newSkillTool())) +} diff --git a/cli/azd/pkg/tool/manifest.go b/cli/azd/pkg/tool/manifest.go index c2bbb553af1..911b6629cc3 100644 --- a/cli/azd/pkg/tool/manifest.go +++ b/cli/azd/pkg/tool/manifest.go @@ -93,9 +93,14 @@ type SkillHost struct { // an existence-only check. BinaryVersionArgs []string // BinaryVersionRegex is a Go regular expression whose first capture - // group matches the host binary's own version (e.g. `\d+\.\d+\.\d+`). - // The installer treats a match as proof the host CLI is genuinely - // installed. When empty, the functional probe is skipped. + // group matches the host binary's own version. To avoid mistaking a + // launcher stub for a real CLI, anchor it to the host's `--version` + // banner with `(?m)^` (e.g. `(?m)^GitHub Copilot CLI\s+v?(\d+\.\d+\.\d+)`) + // rather than matching a bare semver: a stub's output may contain an + // incidental version-shaped token (a bundled runtime version, a path + // build number, a URL) that must not count. The installer treats a match + // against the probe output as proof the host CLI is genuinely installed. + // When empty, the functional probe is skipped. BinaryVersionRegex string } @@ -372,8 +377,10 @@ func azureSkills() *ToolDefinition { VersionRegex: `azure@azure-skills[^\n]*?(\d+\.\d+\.\d+)`, // Probe the host binary itself so a launcher stub that only // prompts to install the real CLI is not mistaken for a host. + // Anchored to copilot's `--version` banner ("GitHub Copilot + // CLI 1.0.64-3") so an incidental semver cannot pass. BinaryVersionArgs: []string{"--version"}, - BinaryVersionRegex: `(\d+\.\d+\.\d+)`, + BinaryVersionRegex: `(?m)^GitHub Copilot CLI\s+v?(\d+\.\d+\.\d+)`, }, { Host: "claude", @@ -391,8 +398,10 @@ func azureSkills() *ToolDefinition { VersionRegex: `Version:\s*v?(\d+\.\d+\.\d+)`, // Probe the host binary itself so a launcher stub that only // prompts to install the real CLI is not mistaken for a host. + // Anchored to claude's `--version` banner ("2.1.178 (Claude + // Code)") so an incidental semver cannot pass. BinaryVersionArgs: []string{"--version"}, - BinaryVersionRegex: `(\d+\.\d+\.\d+)`, + BinaryVersionRegex: `(?m)^v?(\d+\.\d+\.\d+)\s+\(Claude Code\)`, }, }, } diff --git a/cli/azd/pkg/tool/manifest_test.go b/cli/azd/pkg/tool/manifest_test.go index 73d69432682..b545a4a440b 100644 --- a/cli/azd/pkg/tool/manifest_test.go +++ b/cli/azd/pkg/tool/manifest_test.go @@ -373,3 +373,62 @@ func TestAllPlatforms(t *testing.T) { assert.Equal(t, strategy, got) } } + +// TestAzureSkillsHostVersionProbeRegex locks the per-host BinaryVersionRegex +// against real `--version` banners: each host's regex must capture the version +// from that host's genuine output and reject non-version output (a launcher +// stub prompt, the banner prefix without a version, or an incidental semver +// elsewhere in the stream). +func TestAzureSkillsHostVersionProbeRegex(t *testing.T) { + t.Parallel() + + rx := map[string]string{} + for _, h := range azureSkills().SkillHosts { + rx[h.Host] = h.BinaryVersionRegex + } + + cases := []struct { + name string + host string + output string + wantVer string // "" => must not match (host treated as unusable) + }{ + { + name: "copilot real banner", + host: "copilot", + output: "GitHub Copilot CLI 1.0.64-3.\nRun 'copilot update' to check for updates.", + wantVer: "1.0.64", + }, + { + name: "claude real banner", + host: "claude", + output: "2.1.178 (Claude Code)", + wantVer: "2.1.178", + }, + { + name: "copilot stub prompt", + host: "copilot", + output: "Cannot find GitHub Copilot CLI (https://docs.github.com/copilot)\nInstall GitHub Copilot CLI? ['y/N']", + wantVer: "", + }, + { + name: "copilot banner prefix without version", + host: "copilot", + output: "GitHub Copilot CLI is not installed\nnode v20.11.1", + wantVer: "", + }, + { + name: "claude version not at line start", + host: "claude", + output: "see https://example.com/1.2.3 (Claude Code plugin)", + wantVer: "", + }, + } + + for _, tc := range cases { + t.Run(tc.name, func(t *testing.T) { + t.Parallel() + assert.Equal(t, tc.wantVer, matchVersion(tc.output, rx[tc.host])) + }) + } +} From 20a17751d00254a8c75a2094008db45bebc1b667 Mon Sep 17 00:00:00 2001 From: hemarina Date: Fri, 26 Jun 2026 16:09:16 -0700 Subject: [PATCH 4/5] adjustment for fix lost in merge --- cli/azd/pkg/tool/installer.go | 18 ++++---- cli/azd/pkg/tool/installer_test.go | 66 ++++++++++++++++++++++++++++++ 2 files changed, 76 insertions(+), 8 deletions(-) diff --git a/cli/azd/pkg/tool/installer.go b/cli/azd/pkg/tool/installer.go index cc483e7a033..c15cc089da2 100644 --- a/cli/azd/pkg/tool/installer.go +++ b/cli/azd/pkg/tool/installer.go @@ -510,7 +510,7 @@ func (i *installer) resolveSkillUninstallTargets( hosts []string, ) ([]SkillHost, error) { if len(hosts) > 0 { - return i.explicitSkillHostTargets(tool, hosts) + return i.explicitSkillHostTargets(ctx, tool, hosts) } // Default / --host all: remove from every host the skill is @@ -1026,28 +1026,30 @@ func (i *installer) resolveSkillTargets( return []SkillHost{host}, nil } - return i.explicitSkillHostTargets(tool, hosts) + return i.explicitSkillHostTargets(ctx, tool, hosts) } // explicitSkillHostTargets resolves an explicit list of requested host // names to their [SkillHost] definitions. A requested host is usable only -// when it is a configured SkillHost that is also on PATH; both an unknown -// name and a known-but-not-on-PATH host fail with an error naming the +// when it is a configured SkillHost that is also a functional CLI on PATH +// (see [installer.hostUsable]); an unknown name, a host not on PATH, and a +// host present only as a launcher stub all fail with an error naming the // supported hosts. Shared by the install/upgrade (resolveSkillTargets) and // uninstall (resolveSkillUninstallTargets) paths so the host-availability // rule lives in one place. func (i *installer) explicitSkillHostTargets( + ctx context.Context, tool *ToolDefinition, hosts []string, ) ([]SkillHost, error) { targets := make([]SkillHost, 0, len(hosts)) for _, name := range hosts { // A requested host is usable only if it is a configured SkillHost - // that is also on PATH. "unknown name" and "known but not on PATH" - // both mean the host can't be used, so we point the user at the - // supported hosts. + // that is a functional CLI on PATH. "unknown name", "not on PATH" + // and "present but a launcher stub" all mean the host can't be + // used, so we point the user at the supported hosts. host, ok := findSkillHost(tool, name) - if !ok || i.commandRunner.ToolInPath(name) != nil { + if !ok || !i.hostUsable(ctx, host) { supported := make([]string, len(tool.SkillHosts)) for j, h := range tool.SkillHosts { supported[j] = h.Host diff --git a/cli/azd/pkg/tool/installer_test.go b/cli/azd/pkg/tool/installer_test.go index 9f7cc263f79..476110242ed 100644 --- a/cli/azd/pkg/tool/installer_test.go +++ b/cli/azd/pkg/tool/installer_test.go @@ -2300,6 +2300,72 @@ func TestRunSkill_ExplicitHostNotPresent_Errors(t *testing.T) { assert.Contains(t, result.Error.Error(), "claude") } +// TestRunSkill_Install_ExplicitStubHost_Rejected is a regression guard for +// the explicit `--host` path: a host requested by name that is present on +// PATH only as a launcher stub (not a functional CLI) must be rejected the +// same way the default / `--host all` path rejects it. explicitSkillHostTargets +// uses [installer.hostUsable] (a version probe), not a bare PATH-existence +// check, so the stub fails with "not available" instead of being driven +// through an install flow that would later surface "verification failed". +func TestRunSkill_Install_ExplicitStubHost_Rejected(t *testing.T) { + t.Parallel() + + runner := mockexec.NewMockCommandRunner() + // copilot is on PATH but is a launcher stub; claude is not on PATH. + runner.MockToolInPath("copilot", nil) + runner.MockToolInPath("claude", errors.New("not found")) + runner.MockToolInPath("node", nil) + + // The stub answers `copilot --version` with its install prompt instead + // of a version banner, and exits 0. Record that the probe ran so the + // test fails loudly if mock precedence ever let a version-shaped + // response through instead. + var stubVersionProbed bool + runner.When(func(args exec.RunArgs, _ string) bool { + return args.Cmd == "copilot" && slices.Contains(args.Args, "--version") + }).RespondFn(func(_ exec.RunArgs) (exec.RunResult, error) { + stubVersionProbed = true + return exec.RunResult{ + Stdout: "Install GitHub Copilot CLI? ['y/N']", + Stderr: "Cannot find GitHub Copilot CLI (https://docs.github.com/copilot)", + ExitCode: 0, + }, nil + }) + + // Fail if any plugin command is attempted through the stub. + var attemptedInstall bool + runner.When(func(args exec.RunArgs, _ string) bool { + return args.Cmd == "copilot" && + (slices.Contains(args.Args, "install") || + slices.Contains(args.Args, "marketplace") || + slices.Contains(args.Args, "list")) + }).RespondFn(func(_ exec.RunArgs) (exec.RunResult, error) { + attemptedInstall = true + return exec.RunResult{ExitCode: 0}, nil + }) + + inst := NewInstaller( + runner, NewPlatformDetector(runner), installedDetector("1.1.70"), + ) + + result, err := inst.Install( + t.Context(), newSkillTool(), WithHosts("copilot"), + ) + require.NoError(t, err) + require.False(t, result.Success) + require.NotNil(t, result.Error) + + // The explicit host is rejected up front, naming the requested host — + // not driven through an install that fails verification. + assert.Contains(t, result.Error.Error(), "not available") + assert.Contains(t, result.Error.Error(), "copilot") + assert.NotContains(t, result.Error.Error(), "verification failed") + assert.False(t, attemptedInstall, + "must not attempt to install through a non-functional launcher stub") + assert.True(t, stubVersionProbed, + "explicitSkillHostTargets must probe `copilot --version` via hostUsable") +} + // TestRunSkill_Upgrade_DetectError_Propagated verifies that a context // cancellation/timeout from DetectSkillHosts on the default upgrade path // is treated as fatal rather than silently falling back to a host. From ba63de0b1663f66b3e1adca57eec3886b8dd87c8 Mon Sep 17 00:00:00 2001 From: hemarina Date: Fri, 26 Jun 2026 17:21:42 -0700 Subject: [PATCH 5/5] clean up comments --- cli/azd/pkg/tool/installer.go | 53 +++++++++++++---------------------- 1 file changed, 19 insertions(+), 34 deletions(-) diff --git a/cli/azd/pkg/tool/installer.go b/cli/azd/pkg/tool/installer.go index c15cc089da2..f8a0b49efe4 100644 --- a/cli/azd/pkg/tool/installer.go +++ b/cli/azd/pkg/tool/installer.go @@ -500,10 +500,10 @@ func (i *installer) runSkillUninstall( // resolveSkillUninstallTargets resolves the host(s) a skill should be // removed from. With explicit host names, each must be a configured -// SkillHost that is on PATH. Without explicit hosts (the default, and -// also `--host all`), it targets every host the skill is currently -// installed through; an error is returned when the skill is not -// installed on any host. +// SkillHost that is a usable (functional) CLI on PATH. Without explicit +// hosts (the default, and also `--host all`), it targets every host the +// skill is currently installed through; an error is returned when the +// skill is not installed on any host. func (i *installer) resolveSkillUninstallTargets( ctx context.Context, tool *ToolDefinition, @@ -1044,10 +1044,6 @@ func (i *installer) explicitSkillHostTargets( ) ([]SkillHost, error) { targets := make([]SkillHost, 0, len(hosts)) for _, name := range hosts { - // A requested host is usable only if it is a configured SkillHost - // that is a functional CLI on PATH. "unknown name", "not on PATH" - // and "present but a launcher stub" all mean the host can't be - // used, so we point the user at the supported hosts. host, ok := findSkillHost(tool, name) if !ok || !i.hostUsable(ctx, host) { supported := make([]string, len(tool.SkillHosts)) @@ -1092,35 +1088,24 @@ func configuredSkillHostsFor(tool *ToolDefinition, installed []InstalledSkillHos return targets } -// hostUsable reports whether an agentic CLI host is a *functional* -// installation rather than merely a file named like the host on PATH. +// hostUsable reports whether an agentic CLI host on PATH is a functional +// CLI rather than a same-named launcher stub. // -// Some environments place a launcher stub on PATH that is not the -// real CLI — most notably the VS Code GitHub Copilot Chat extension, which -// drops a small `copilot` stub into its globalStorage and adds that folder -// to the integrated terminal's PATH. Invoking the stub only prints -// "Install GitHub Copilot CLI?" and exits 0 without doing anything, so it -// satisfies a bare exec.LookPath existence check yet cannot install the -// skill — which previously surfaced as the misleading -// " was installed via but verification failed". +// Some environments put a stub on PATH — notably the VS Code Copilot Chat +// extension, whose `copilot` stub only prints "Install GitHub Copilot CLI?" +// and exits 0. It passes a bare existence check but cannot install the skill, +// which used to surface as a misleading "verification failed". // -// hostUsable shells out to the host's version command (non-interactively, -// with an empty stdin so a stub that prompts reads EOF and exits instead of -// hanging) and treats the host as usable only when the output matches the -// host's BinaryVersionRegex. That regex is anchored to the host's own -// `--version` banner (e.g. "^GitHub Copilot CLI "), so an incidental -// version-shaped token elsewhere in a stub's output does not count. Hosts -// that do not configure a version probe (BinaryVersionArgs/BinaryVersionRegex) -// fall back to the existence check. +// To tell them apart, hostUsable runs the host's version command (with empty +// stdin so a prompting stub reads EOF and exits) and accepts the host only +// when the output matches its BinaryVersionRegex, anchored to the host's +// `--version` banner. Hosts without a version probe fall back to the +// existence check. // -// hostUsable is consulted from several host-resolution call sites within one -// command, so the version-probe result is memoized per host (hostProbe). Only -// on-PATH results are cached: a host that is not on PATH is re-checked every -// time (the exec.LookPath check is cheap) so a host installed earlier in the -// same batch — e.g. `azd tool install github-copilot-cli azure-skills` — is -// still picked up at run time. The cache assumes an on-PATH host binary is not -// swapped in place mid-command, which azd never does (it installs hosts to -// their own package locations). +// Results are memoized per host (hostProbe); only on-PATH hosts are cached, +// so a host installed earlier in the same batch is still picked up. The cache +// assumes an on-PATH host binary is not swapped mid-command, which azd never +// does. func (i *installer) hostUsable(ctx context.Context, host SkillHost) bool { if i.commandRunner.ToolInPath(host.Host) != nil { return false