From 998b6212b38f653f997c79fc42e6f13197008581 Mon Sep 17 00:00:00 2001 From: William Martin Date: Fri, 17 Apr 2026 19:58:59 +0200 Subject: [PATCH] Add skills specific telemetry MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * Add skills specific telemetry * Remove VisibilityFuture, inline goroutine at call sites The VisibilityFuture/FetchRepoVisibilityAsync/Wait wrapper was an unidiomatic async abstraction built for a single pattern used in exactly two call sites. In Go the channel is already the future; wrapping it in a struct with a Wait(timeout) method adds no value. Delete the abstraction and inline a local visResult struct, buffered channel, goroutine, and select at each call site. Behavior is preserved exactly: err -> "unknown", timeout -> "unknown", success+public -> include skill_names. FetchRepoVisibility (synchronous) is kept as-is. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> * Fix nonsense copilot tests * Update telemetry tests for public-only dims and search event removal Production telemetry emission changed: - preview: skill_owner/skill_repo/skill_name (renamed from skill_names) are now emitted only when repo_visibility=public. - install: skill_owner/skill_repo/skill_names are now emitted only when repo_visibility=public. - search: the initial skill_search event was removed entirely; the skill_search_install event no longer carries query/owner dims. Update tests to match: rename skill_names -> skill_name in preview, make owner/repo assertions conditional on public visibility in both preview and install, and reduce the search test to a single event with explicit Empty assertions for the removed query/owner dims so a privacy regression cannot pass silently. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> * Test CategorizeHost and switch telemetry to skill_host_type Add TestCategorizeHost covering all four classification branches (github.com, ghes, tenancy, uncategorized) with cases verified against the real ghauth implementation rather than guessed. Update install and preview unit tests to assert the new skill_host_type dimension name, and fix a typo in the preview acceptance txtar (skill_hos_type -> skill_host_type). Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> * Shrink visibility wait and test unknown visibility The 2s visibilityWaitTimeout was wildly overprovisioned: by the time telemetry emission reaches the select, the command has already done several serial GitHub REST calls (and for install, a git sparse-checkout plus possibly interactive prompts), so the one-call visibility fetch has almost always completed. Drop the timeout to 200ms — a short safety net for a stalled REST call, not a wait budget for a healthy one. Also adds a table-driven case to TestFetchRepoVisibility covering an unknown/future visibility value from the API, addressing @babakks' review nitpick. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --------- Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- .../testdata/skills/skills-install.txtar | 13 + .../testdata/skills/skills-preview.txtar | 12 + .../skills/skills-publish-dry-run.txtar | 4 - .../testdata/skills/skills-search-page.txtar | 2 +- .../testdata/skills/skills-search.txtar | 4 +- internal/ghinstance/host.go | 16 ++ internal/ghinstance/host_test.go | 54 +++++ internal/skills/discovery/discovery.go | 71 ++++-- internal/skills/discovery/discovery_test.go | 80 +++++++ internal/telemetry/fake.go | 20 ++ pkg/cmd/root/root.go | 2 +- pkg/cmd/skills/install/install.go | 68 +++++- pkg/cmd/skills/install/install_test.go | 188 ++++++++++++++- pkg/cmd/skills/preview/preview.go | 67 +++++- pkg/cmd/skills/preview/preview_test.go | 223 +++++++++++++++++- pkg/cmd/skills/search/search.go | 12 +- pkg/cmd/skills/search/search_test.go | 77 +++++- pkg/cmd/skills/skills.go | 13 +- pkg/cmd/skills/skills_test.go | 19 ++ 19 files changed, 896 insertions(+), 49 deletions(-) create mode 100644 pkg/cmd/skills/skills_test.go diff --git a/acceptance/testdata/skills/skills-install.txtar b/acceptance/testdata/skills/skills-install.txtar index c365cb833..0311a0db2 100644 --- a/acceptance/testdata/skills/skills-install.txtar +++ b/acceptance/testdata/skills/skills-install.txtar @@ -18,3 +18,16 @@ stdout 'Installed git-commit' # Verify the skill was written to the custom directory exists $WORK/custom-skills/git-commit/SKILL.md grep 'github-repo' $WORK/custom-skills/git-commit/SKILL.md + +# Telemetry: skill_install event records agent hosts, repo identifiers, +# and (for a public repo) the installed skill name. +env GH_PRIVATE_ENABLE_TELEMETRY=1 +env GH_TELEMETRY=log +env GH_TELEMETRY_SAMPLE_RATE=100 +exec gh skill install github/awesome-copilot git-commit --scope user --force --agent github-copilot +stderr 'Telemetry payload:' +stderr '"type": "skill_install"' +stderr '"agent_hosts": "github-copilot"' +stderr '"skill_host_type": "github.com"' +stderr '"skill_owner": "github"' +stderr '"skill_repo": "awesome-copilot"' diff --git a/acceptance/testdata/skills/skills-preview.txtar b/acceptance/testdata/skills/skills-preview.txtar index be1be5244..af1d0bbbe 100644 --- a/acceptance/testdata/skills/skills-preview.txtar +++ b/acceptance/testdata/skills/skills-preview.txtar @@ -7,3 +7,15 @@ stdout 'git-commit/' # Preview a skill that doesn't exist should error ! exec gh skill preview github/awesome-copilot nonexistent-skill-xyz stderr 'not found' + +# Telemetry: skill_preview event records repo identifiers and, for a +# public repo, the skill name. +env GH_PRIVATE_ENABLE_TELEMETRY=1 +env GH_TELEMETRY=log +env GH_TELEMETRY_SAMPLE_RATE=100 +exec gh skill preview github/awesome-copilot git-commit +stderr 'Telemetry payload:' +stderr '"type": "skill_preview"' +stderr '"skill_host_type": "github.com"' +stderr '"skill_owner": "github"' +stderr '"skill_repo": "awesome-copilot"' diff --git a/acceptance/testdata/skills/skills-publish-dry-run.txtar b/acceptance/testdata/skills/skills-publish-dry-run.txtar index cb32fa7e2..786204951 100644 --- a/acceptance/testdata/skills/skills-publish-dry-run.txtar +++ b/acceptance/testdata/skills/skills-publish-dry-run.txtar @@ -6,10 +6,6 @@ stderr 'no skills found in' exec gh skill publish --dry-run $WORK/test-repo stdout 'hello-world' -# Validate alias should work identically -exec gh skill validate --dry-run $WORK/test-repo -stdout 'hello-world' - # Publish dry-run with --tag exec gh skill publish --dry-run --tag v1.0.0 $WORK/test-repo stdout 'hello-world' diff --git a/acceptance/testdata/skills/skills-search-page.txtar b/acceptance/testdata/skills/skills-search-page.txtar index 30c044f78..48409c235 100644 --- a/acceptance/testdata/skills/skills-search-page.txtar +++ b/acceptance/testdata/skills/skills-search-page.txtar @@ -1,3 +1,3 @@ # Pagination returns results on page 2 -exec gh skill search copilot --page 2 +exec gh skill search --owner github copilot --page 2 stdout 'copilot' diff --git a/acceptance/testdata/skills/skills-search.txtar b/acceptance/testdata/skills/skills-search.txtar index 5e8c77442..e16936b0d 100644 --- a/acceptance/testdata/skills/skills-search.txtar +++ b/acceptance/testdata/skills/skills-search.txtar @@ -1,5 +1,5 @@ # Search for skills matching a query -exec gh skill search copilot +exec gh skill search --owner github copilot stdout 'copilot' # Search with JSON output @@ -9,4 +9,4 @@ stdout '"repo"' # Search with a short query should error ! exec gh skill search a -stderr 'at least' +stderr 'at least' \ No newline at end of file diff --git a/internal/ghinstance/host.go b/internal/ghinstance/host.go index 7abfd83ac..dfea6dd94 100644 --- a/internal/ghinstance/host.go +++ b/internal/ghinstance/host.go @@ -96,3 +96,19 @@ func HostPrefix(hostname string) string { } return fmt.Sprintf("https://%s/", hostname) } + +func CategorizeHost(host string) string { + if host == defaultHostname { + return "github.com" + } + + if ghauth.IsEnterprise(host) { + return "ghes" + } + + if ghauth.IsTenancy(host) { + return "tenancy" + } + + return "uncategorized" +} diff --git a/internal/ghinstance/host_test.go b/internal/ghinstance/host_test.go index 1b7e0146d..c4f447780 100644 --- a/internal/ghinstance/host_test.go +++ b/internal/ghinstance/host_test.go @@ -157,3 +157,57 @@ func TestRESTPrefix(t *testing.T) { }) } } + +func TestCategorizeHost(t *testing.T) { + tests := []struct { + name string + host string + want string + }{ + { + name: "github.com returns github.com", + host: "github.com", + want: "github.com", + }, + { + name: "classic GHES hostname returns ghes", + host: "ghe.io", + want: "ghes", + }, + { + name: "arbitrary enterprise hostname returns ghes", + host: "enterprise.example.com", + want: "ghes", + }, + { + name: "tenant subdomain of ghe.com returns tenancy", + host: "tenant.ghe.com", + want: "tenancy", + }, + { + name: "api subdomain under tenant returns tenancy", + host: "api.tenant.ghe.com", + want: "tenancy", + }, + { + name: "bare ghe.com returns ghes", + host: "ghe.com", + want: "ghes", + }, + { + name: "github.localhost returns uncategorized", + host: "github.localhost", + want: "uncategorized", + }, + { + name: "github.com subdomain returns uncategorized", + host: "garage.github.com", + want: "uncategorized", + }, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + assert.Equal(t, tt.want, CategorizeHost(tt.host)) + }) + } +} diff --git a/internal/skills/discovery/discovery.go b/internal/skills/discovery/discovery.go index 93de67faa..2d6c1ee72 100644 --- a/internal/skills/discovery/discovery.go +++ b/internal/skills/discovery/discovery.go @@ -126,18 +126,37 @@ type treeResponse struct { Truncated bool `json:"truncated"` } -type blobResponse struct { - SHA string `json:"sha"` - Content string `json:"content"` - Encoding string `json:"encoding"` +type RepoVisibility string + +const ( + RepoVisibilityPublic RepoVisibility = "public" + RepoVisibilityPrivate RepoVisibility = "private" + RepoVisibilityInternal RepoVisibility = "internal" +) + +func parseRepoVisibility(s string) (RepoVisibility, error) { + switch s { + case "public": + return RepoVisibilityPublic, nil + case "private": + return RepoVisibilityPrivate, nil + case "internal": + return RepoVisibilityInternal, nil + default: + return "", fmt.Errorf("unknown repository visibility: %q", s) + } } -type releaseResponse struct { - TagName string `json:"tag_name"` -} - -type repoResponse struct { - DefaultBranch string `json:"default_branch"` +// FetchRepoVisibility returns the repository visibility: "public", "private", or "internal". +func FetchRepoVisibility(client *api.Client, host, owner, repo string) (RepoVisibility, error) { + apiPath := fmt.Sprintf("repos/%s/%s", url.PathEscape(owner), url.PathEscape(repo)) + var resp struct { + Visibility string `json:"visibility"` + } + if err := client.REST(host, "GET", apiPath, nil, &resp); err != nil { + return "", err + } + return parseRepoVisibility(resp.Visibility) } // ResolveRef determines the git ref to use for a given owner/repo. @@ -266,8 +285,10 @@ func (e *noReleasesError) Error() string { return e.reason } func resolveLatestRelease(client *api.Client, host, owner, repo string) (*ResolvedRef, error) { apiPath := fmt.Sprintf("repos/%s/%s/releases/latest", url.PathEscape(owner), url.PathEscape(repo)) - var release releaseResponse - if err := client.REST(host, "GET", apiPath, nil, &release); err != nil { + var resp struct { + TagName string `json:"tag_name"` + } + if err := client.REST(host, "GET", apiPath, nil, &resp); err != nil { // A 404 means the repository has no releases. This is the // only case where falling back to the default branch is safe. // Any other HTTP error (403, 500, …) or network failure is @@ -278,19 +299,21 @@ func resolveLatestRelease(client *api.Client, host, owner, repo string) (*Resolv } return nil, fmt.Errorf("could not fetch latest release: %w", err) } - if release.TagName == "" { + if resp.TagName == "" { return nil, &noReleasesError{reason: "latest release has no tag"} } - return resolveTagRef(client, host, owner, repo, release.TagName) + return resolveTagRef(client, host, owner, repo, resp.TagName) } func resolveDefaultBranch(client *api.Client, host, owner, repo string) (*ResolvedRef, error) { apiPath := fmt.Sprintf("repos/%s/%s", url.PathEscape(owner), url.PathEscape(repo)) - var repoResp repoResponse - if err := client.REST(host, "GET", apiPath, nil, &repoResp); err != nil { + var resp struct { + DefaultBranch string `json:"default_branch"` + } + if err := client.REST(host, "GET", apiPath, nil, &resp); err != nil { return nil, fmt.Errorf("could not determine default branch: %w", err) } - branch := repoResp.DefaultBranch + branch := resp.DefaultBranch if branch == "" { return nil, fmt.Errorf("could not determine default branch for %s/%s", owner, repo) } @@ -657,18 +680,22 @@ func walkTree(client *api.Client, host, owner, repo, sha, prefix string, depth i // FetchBlob retrieves the content of a blob by SHA. func FetchBlob(client *api.Client, host, owner, repo, sha string) (string, error) { apiPath := fmt.Sprintf("repos/%s/%s/git/blobs/%s", url.PathEscape(owner), url.PathEscape(repo), url.PathEscape(sha)) - var blob blobResponse - if err := client.REST(host, "GET", apiPath, nil, &blob); err != nil { + var resp struct { + SHA string `json:"sha"` + Content string `json:"content"` + Encoding string `json:"encoding"` + } + if err := client.REST(host, "GET", apiPath, nil, &resp); err != nil { return "", fmt.Errorf("could not fetch blob: %w", err) } - if blob.Encoding != "base64" { - return "", fmt.Errorf("unexpected blob encoding: %s", blob.Encoding) + if resp.Encoding != "base64" { + return "", fmt.Errorf("unexpected blob encoding: %s", resp.Encoding) } // GitHub API returns base64 with embedded newlines; use the StdEncoding // decoder via a reader to handle them transparently. - decoded, err := io.ReadAll(base64.NewDecoder(base64.StdEncoding, strings.NewReader(blob.Content))) + decoded, err := io.ReadAll(base64.NewDecoder(base64.StdEncoding, strings.NewReader(resp.Content))) if err != nil { return "", fmt.Errorf("could not decode blob content: %w", err) } diff --git a/internal/skills/discovery/discovery_test.go b/internal/skills/discovery/discovery_test.go index f9abc593c..8929e1787 100644 --- a/internal/skills/discovery/discovery_test.go +++ b/internal/skills/discovery/discovery_test.go @@ -561,6 +561,86 @@ func TestFetchBlob(t *testing.T) { } } +func TestFetchRepoVisibility(t *testing.T) { + tests := []struct { + name string + stubs func(*httpmock.Registry) + want RepoVisibility + wantErr string + }{ + { + name: "public repo", + stubs: func(reg *httpmock.Registry) { + reg.Register( + httpmock.REST("GET", "repos/monalisa/octocat-skills"), + httpmock.JSONResponse(map[string]interface{}{ + "visibility": "public", + })) + }, + want: RepoVisibilityPublic, + }, + { + name: "private repo", + stubs: func(reg *httpmock.Registry) { + reg.Register( + httpmock.REST("GET", "repos/monalisa/octocat-skills"), + httpmock.JSONResponse(map[string]interface{}{ + "visibility": "private", + })) + }, + want: RepoVisibilityPrivate, + }, + { + name: "internal repo", + stubs: func(reg *httpmock.Registry) { + reg.Register( + httpmock.REST("GET", "repos/monalisa/octocat-skills"), + httpmock.JSONResponse(map[string]interface{}{ + "visibility": "internal", + })) + }, + want: RepoVisibilityInternal, + }, + { + name: "unknown visibility", + stubs: func(reg *httpmock.Registry) { + reg.Register( + httpmock.REST("GET", "repos/monalisa/octocat-skills"), + httpmock.JSONResponse(map[string]interface{}{ + "visibility": "cool-visibility", + })) + }, + wantErr: `unknown repository visibility: "cool-visibility"`, + }, + { + name: "API error", + stubs: func(reg *httpmock.Registry) { + reg.Register( + httpmock.REST("GET", "repos/monalisa/octocat-skills"), + httpmock.StatusStringResponse(500, "server error")) + }, + wantErr: "HTTP 500", + }, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + reg := &httpmock.Registry{} + defer reg.Verify(t) + tt.stubs(reg) + client := api.NewClientFromHTTP(&http.Client{Transport: reg}) + + got, err := FetchRepoVisibility(client, "github.com", "monalisa", "octocat-skills") + if tt.wantErr != "" { + require.Error(t, err) + assert.Contains(t, err.Error(), tt.wantErr) + return + } + require.NoError(t, err) + assert.Equal(t, tt.want, got) + }) + } +} + func TestDiscoverSkills(t *testing.T) { tests := []struct { name string diff --git a/internal/telemetry/fake.go b/internal/telemetry/fake.go index 1dc45ab26..4eb22e898 100644 --- a/internal/telemetry/fake.go +++ b/internal/telemetry/fake.go @@ -13,3 +13,23 @@ func (r *EventRecorderSpy) Record(event ghtelemetry.Event) { func (r *EventRecorderSpy) Disable() {} func (r *EventRecorderSpy) Flush() {} + +// CommandRecorderSpy is a test double for ghtelemetry.CommandRecorder. +// It captures recorded events and the most recent SetSampleRate call so tests can +// assert on the sampling behavior commands attempt to configure. +type CommandRecorderSpy struct { + Events []ghtelemetry.Event + LastSampleRate int +} + +func (r *CommandRecorderSpy) Record(event ghtelemetry.Event) { + r.Events = append(r.Events, event) +} + +func (r *CommandRecorderSpy) Disable() {} + +func (r *CommandRecorderSpy) SetSampleRate(rate int) { + r.LastSampleRate = rate +} + +func (r *CommandRecorderSpy) Flush() {} diff --git a/pkg/cmd/root/root.go b/pkg/cmd/root/root.go index 7df9d2986..9f4fa6f5b 100644 --- a/pkg/cmd/root/root.go +++ b/pkg/cmd/root/root.go @@ -149,7 +149,7 @@ func NewCmdRoot(f *cmdutil.Factory, telemetry ghtelemetry.CommandRecorder, versi cmd.AddCommand(codespaceCmd.NewCmdCodespace(f)) cmd.AddCommand(projectCmd.NewCmdProject(f)) cmd.AddCommand(previewCmd.NewCmdPreview(f)) - cmd.AddCommand(skillsCmd.NewCmdSkills(f)) + cmd.AddCommand(skillsCmd.NewCmdSkills(f, telemetry)) // Root commands with standalone functionality and no subcommands cmd.AddCommand(copilotCmd.NewCmdCopilot(f, nil)) diff --git a/pkg/cmd/skills/install/install.go b/pkg/cmd/skills/install/install.go index 1b6a7fd8f..d4a05440e 100644 --- a/pkg/cmd/skills/install/install.go +++ b/pkg/cmd/skills/install/install.go @@ -8,11 +8,14 @@ import ( "os" "path/filepath" "strings" + "time" "github.com/MakeNowJust/heredoc" "github.com/cli/cli/v2/api" ghContext "github.com/cli/cli/v2/context" "github.com/cli/cli/v2/git" + "github.com/cli/cli/v2/internal/gh/ghtelemetry" + "github.com/cli/cli/v2/internal/ghinstance" "github.com/cli/cli/v2/internal/ghrepo" "github.com/cli/cli/v2/internal/prompter" "github.com/cli/cli/v2/internal/skills/discovery" @@ -38,6 +41,7 @@ const ( // InstallOptions holds all dependencies and user-provided flags for the install command. type InstallOptions struct { IO *iostreams.IOStreams + Telemetry ghtelemetry.EventRecorder HttpClient func() (*http.Client, error) Prompter prompter.Prompter GitClient *git.Client @@ -59,9 +63,10 @@ type InstallOptions struct { } // NewCmdInstall creates the "skills install" command. -func NewCmdInstall(f *cmdutil.Factory, runF func(*InstallOptions) error) *cobra.Command { +func NewCmdInstall(f *cmdutil.Factory, telemetry ghtelemetry.CommandRecorder, runF func(*InstallOptions) error) *cobra.Command { opts := &InstallOptions{ IO: f.IOStreams, + Telemetry: telemetry, Prompter: f.Prompter, GitClient: f.GitClient, Remotes: f.Remotes, @@ -232,6 +237,19 @@ func installRun(opts *InstallOptions) error { return err } + // Kick off the visibility fetch in parallel with the install work so + // the extra API roundtrip doesn't add latency on the critical path. + // The result is consumed when the telemetry event is emitted below. + type visResult struct { + vis discovery.RepoVisibility + err error + } + visCh := make(chan visResult, 1) + go func() { + vis, err := discovery.FetchRepoVisibility(apiClient, hostname, opts.repo.RepoOwner(), opts.repo.RepoName()) + visCh <- visResult{vis: vis, err: err} + }() + resolved, err := resolveVersion(opts, apiClient, hostname) if err != nil { return err @@ -325,9 +343,57 @@ func installRun(opts *InstallOptions) error { } } + dims := map[string]string{ + "agent_hosts": mapAgentHostsToIDs(selectedHosts), + "skill_host_type": ghinstance.CategorizeHost(opts.repo.RepoHost()), + } + select { + case r := <-visCh: + if r.err == nil { + dims["repo_visibility"] = string(r.vis) + if r.vis == discovery.RepoVisibilityPublic { + dims["skill_owner"] = opts.repo.RepoOwner() + dims["skill_repo"] = opts.repo.RepoName() + dims["skill_names"] = mapSkillsToNames(selectedSkills) + } + } else { + dims["repo_visibility"] = "unknown" + } + case <-time.After(visibilityWaitTimeout): + dims["repo_visibility"] = "unknown" + } + opts.Telemetry.Record(ghtelemetry.Event{ + Type: "skill_install", + Dimensions: dims, + }) + return nil } +// visibilityWaitTimeout is how long to wait at telemetry-emit time for +// the in-flight repo visibility fetch before giving up and emitting +// repo_visibility="unknown". By this point the command has already done +// several serial API calls and (for install) a git sparse-checkout, so +// the fetch has almost always completed; this budget is a short safety +// net for the case where that single REST call has stalled. +const visibilityWaitTimeout = 200 * time.Millisecond + +func mapSkillsToNames(skills []discovery.Skill) string { + names := make([]string, len(skills)) + for i, s := range skills { + names[i] = s.DisplayName() + } + return strings.Join(names, ",") +} + +func mapAgentHostsToIDs(hosts []*registry.AgentHost) string { + agentHostIDs := make([]string, len(hosts)) + for i, h := range hosts { + agentHostIDs[i] = h.ID + } + return strings.Join(agentHostIDs, ",") +} + // runLocalInstall handles installation from a local directory path. func runLocalInstall(opts *InstallOptions) error { cs := opts.IO.ColorScheme() diff --git a/pkg/cmd/skills/install/install_test.go b/pkg/cmd/skills/install/install_test.go index 481227524..0560625f7 100644 --- a/pkg/cmd/skills/install/install_test.go +++ b/pkg/cmd/skills/install/install_test.go @@ -16,6 +16,7 @@ import ( "github.com/cli/cli/v2/internal/ghrepo" "github.com/cli/cli/v2/internal/prompter" "github.com/cli/cli/v2/internal/skills/discovery" + "github.com/cli/cli/v2/internal/telemetry" "github.com/cli/cli/v2/pkg/cmdutil" "github.com/cli/cli/v2/pkg/httpmock" "github.com/cli/cli/v2/pkg/iostreams" @@ -128,7 +129,7 @@ func TestNewCmdInstall(t *testing.T) { } var gotOpts *InstallOptions - cmd := NewCmdInstall(f, func(opts *InstallOptions) error { + cmd := NewCmdInstall(f, &telemetry.NoOpService{}, func(opts *InstallOptions) error { gotOpts = opts return nil }) @@ -167,7 +168,7 @@ func TestNewCmdInstall(t *testing.T) { t.Run("command metadata", func(t *testing.T) { ios, _, _, _ := iostreams.Test() f := &cmdutil.Factory{IOStreams: ios, Prompter: &prompter.PrompterMock{}, GitClient: &git.Client{}} - cmd := NewCmdInstall(f, nil) + cmd := NewCmdInstall(f, &telemetry.NoOpService{}, nil) assert.Equal(t, "install [] [flags]", cmd.Use) assert.NotEmpty(t, cmd.Short) @@ -1348,6 +1349,9 @@ func TestInstallRun(t *testing.T) { ios.SetStdinTTY(tt.isTTY) ios.SetStderrTTY(tt.isTTY) opts := tt.opts(ios, reg) + if opts.Telemetry == nil { + opts.Telemetry = &telemetry.NoOpService{} + } err := installRun(opts) @@ -1414,6 +1418,7 @@ func TestInstallRun_DeduplicatesSharedProjectDirAcrossHosts(t *testing.T) { SkillSource: "monalisa/octocat-skills", SkillName: "git-commit", Force: true, + Telemetry: &telemetry.NoOpService{}, }) require.NoError(t, err) assert.Equal(t, 1, strings.Count(stdout.String(), "Installed git-commit")) @@ -1980,3 +1985,182 @@ func Test_selectSkillsWithSelector_noDisclaimer(t *testing.T) { require.NoError(t, err) assert.NotContains(t, stderr.String(), "not verified by GitHub") } + +func TestInstallRun_TelemetryVisibility(t *testing.T) { + tests := []struct { + name string + visibility string + visibilityErr bool + wantSkillNames string + }{ + { + name: "public repo includes skill names", + visibility: "public", + wantSkillNames: "git-commit", + }, + { + name: "private repo excludes skill names", + visibility: "private", + }, + { + name: "internal repo excludes skill names", + visibility: "internal", + }, + { + name: "API error omits visibility and skill names", + visibilityErr: true, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + reg := &httpmock.Registry{} + defer reg.Verify(t) + + stubResolveVersion(reg, "monalisa", "octocat-skills", "v1.0.0", "abc123") + stubDiscoverTree(reg, "monalisa", "octocat-skills", "abc123", + singleSkillTreeJSON("git-commit", "treeSHA", "blobSHA")) + stubInstallFiles(reg, "monalisa", "octocat-skills", "treeSHA", "blobSHA", gitCommitContent) + if tt.visibilityErr { + reg.Register( + httpmock.REST("GET", "repos/monalisa/octocat-skills"), + httpmock.StatusStringResponse(500, "server error"), + ) + } else { + reg.Register( + httpmock.REST("GET", "repos/monalisa/octocat-skills"), + httpmock.JSONResponse(map[string]interface{}{ + "visibility": tt.visibility, + }), + ) + } + + ios, _, _, _ := iostreams.Test() + ios.SetStdoutTTY(true) + ios.SetStdinTTY(true) + + recorder := &telemetry.EventRecorderSpy{} + + err := installRun(&InstallOptions{ + IO: ios, + HttpClient: func() (*http.Client, error) { return &http.Client{Transport: reg}, nil }, + GitClient: &git.Client{RepoDir: t.TempDir()}, + Prompter: &prompter.PrompterMock{}, + SkillSource: "monalisa/octocat-skills", + SkillName: "git-commit", + Agent: "github-copilot", + Scope: "project", + ScopeChanged: true, + Dir: t.TempDir(), + Force: true, + Telemetry: recorder, + }) + require.NoError(t, err) + + require.Len(t, recorder.Events, 1) + event := recorder.Events[0] + assert.Equal(t, "skill_install", event.Type) + assert.NotEmpty(t, event.Dimensions["agent_hosts"], "agent_hosts should always be present") + + // skill_host_type is always recorded (categorized, no raw hostname for enterprise/tenancy). + assert.Equal(t, "github.com", event.Dimensions["skill_host_type"]) + + if tt.visibilityErr { + assert.Equal(t, "unknown", event.Dimensions["repo_visibility"], + "visibility fetch errors should emit repo_visibility=\"unknown\" so the fallback is distinguishable from a successful fetch") + } else { + assert.Equal(t, tt.visibility, event.Dimensions["repo_visibility"]) + } + + // Owner, repo, and skill names are only included when the repo + // is public; for private/internal/unknown they are omitted to + // avoid leaking identifiers of non-public repositories. + if tt.wantSkillNames != "" { + assert.Equal(t, "monalisa", event.Dimensions["skill_owner"]) + assert.Equal(t, "octocat-skills", event.Dimensions["skill_repo"]) + assert.Equal(t, tt.wantSkillNames, event.Dimensions["skill_names"]) + } else { + assert.Empty(t, event.Dimensions["skill_owner"]) + assert.Empty(t, event.Dimensions["skill_repo"]) + assert.Empty(t, event.Dimensions["skill_names"]) + } + }) + } +} + +func TestInstallRun_TelemetryMultipleSkills(t *testing.T) { + codeReviewContent := heredoc.Doc(` + --- + name: code-review + description: Reviews code + --- + # Code Review + `) + + reg := &httpmock.Registry{} + defer reg.Verify(t) + + stubResolveVersion(reg, "monalisa", "octocat-skills", "v1.0.0", "abc123") + treeJSON := `{"path": "skills/git-commit", "type": "tree", "sha": "treeGC"}, ` + + `{"path": "skills/git-commit/SKILL.md", "type": "blob", "sha": "blobGC"}, ` + + `{"path": "skills/code-review", "type": "tree", "sha": "treeCR"}, ` + + `{"path": "skills/code-review/SKILL.md", "type": "blob", "sha": "blobCR"}` + stubDiscoverTree(reg, "monalisa", "octocat-skills", "abc123", treeJSON) + + // Blob stubs for FetchDescriptionsConcurrent during interactive selection + encGC := base64.StdEncoding.EncodeToString([]byte(gitCommitContent)) + encCR := base64.StdEncoding.EncodeToString([]byte(codeReviewContent)) + reg.Register( + httpmock.REST("GET", "repos/monalisa/octocat-skills/git/blobs/blobGC"), + httpmock.StringResponse(fmt.Sprintf(`{"sha": "blobGC", "content": %q, "encoding": "base64"}`, encGC))) + reg.Register( + httpmock.REST("GET", "repos/monalisa/octocat-skills/git/blobs/blobCR"), + httpmock.StringResponse(fmt.Sprintf(`{"sha": "blobCR", "content": %q, "encoding": "base64"}`, encCR))) + + stubInstallFiles(reg, "monalisa", "octocat-skills", "treeGC", "blobGC", gitCommitContent) + stubInstallFiles(reg, "monalisa", "octocat-skills", "treeCR", "blobCR", codeReviewContent) + + reg.Register( + httpmock.REST("GET", "repos/monalisa/octocat-skills"), + httpmock.JSONResponse(map[string]interface{}{ + "visibility": "public", + }), + ) + + ios, _, _, _ := iostreams.Test() + ios.SetStdoutTTY(true) + ios.SetStdinTTY(true) + + pm := &prompter.PrompterMock{ + MultiSelectWithSearchFunc: func(_, _ string, _, _ []string, _ func(string) prompter.MultiSelectSearchResult) ([]string, error) { + return []string{allSkillsKey}, nil + }, + } + + recorder := &telemetry.EventRecorderSpy{} + + err := installRun(&InstallOptions{ + IO: ios, + HttpClient: func() (*http.Client, error) { return &http.Client{Transport: reg}, nil }, + GitClient: &git.Client{RepoDir: t.TempDir()}, + Prompter: pm, + SkillSource: "monalisa/octocat-skills", + Agent: "github-copilot", + Scope: "project", + ScopeChanged: true, + Dir: t.TempDir(), + Telemetry: recorder, + }) + require.NoError(t, err) + + require.Len(t, recorder.Events, 1) + event := recorder.Events[0] + assert.Equal(t, "skill_install", event.Type) + assert.Equal(t, "public", event.Dimensions["repo_visibility"]) + + // Verify comma-separated skill names (alphabetical order from DiscoverSkills) + names := strings.Split(event.Dimensions["skill_names"], ",") + assert.Len(t, names, 2) + assert.Contains(t, names, "code-review") + assert.Contains(t, names, "git-commit") +} diff --git a/pkg/cmd/skills/preview/preview.go b/pkg/cmd/skills/preview/preview.go index 4bdfdd416..e9f1e0442 100644 --- a/pkg/cmd/skills/preview/preview.go +++ b/pkg/cmd/skills/preview/preview.go @@ -7,9 +7,12 @@ import ( "path" "sort" "strings" + "time" "github.com/MakeNowJust/heredoc" "github.com/cli/cli/v2/api" + "github.com/cli/cli/v2/internal/gh/ghtelemetry" + "github.com/cli/cli/v2/internal/ghinstance" "github.com/cli/cli/v2/internal/ghrepo" "github.com/cli/cli/v2/internal/prompter" "github.com/cli/cli/v2/internal/skills/discovery" @@ -23,6 +26,7 @@ import ( type PreviewOptions struct { IO *iostreams.IOStreams + Telemetry ghtelemetry.EventRecorder HttpClient func() (*http.Client, error) Prompter prompter.Prompter ExecutablePath string @@ -36,9 +40,10 @@ type PreviewOptions struct { } // NewCmdPreview creates the "skills preview" command. -func NewCmdPreview(f *cmdutil.Factory, runF func(*PreviewOptions) error) *cobra.Command { +func NewCmdPreview(f *cmdutil.Factory, telemetry ghtelemetry.CommandRecorder, runF func(*PreviewOptions) error) *cobra.Command { opts := &PreviewOptions{ IO: f.IOStreams, + Telemetry: telemetry, HttpClient: f.HttpClient, Prompter: f.Prompter, ExecutablePath: f.ExecutablePath, @@ -125,6 +130,19 @@ func previewRun(opts *PreviewOptions) error { } apiClient := api.NewClientFromHTTP(httpClient) + // Kick off the visibility fetch in parallel with the preview work so + // the extra API roundtrip doesn't add latency on the critical path. + // The result is consumed when the telemetry event is emitted below. + type visResult struct { + vis discovery.RepoVisibility + err error + } + visCh := make(chan visResult, 1) + go func() { + vis, err := discovery.FetchRepoVisibility(apiClient, hostname, owner, repoName) + visCh <- visResult{vis: vis, err: err} + }() + opts.IO.StartProgressIndicatorWithLabel(fmt.Sprintf("Resolving %s/%s", owner, repoName)) resolved, err := discovery.ResolveRef(apiClient, hostname, owner, repoName, opts.Version) opts.IO.StopProgressIndicator() @@ -177,17 +195,50 @@ func previewRun(opts *PreviewOptions) error { // Non-interactive or skill has only SKILL.md: dump through pager if !canPrompt || len(extraFiles) == 0 { - return renderAllFiles(opts, cs, skill, files, rendered, extraFiles, apiClient, hostname, owner, repoName) + renderAllFiles(opts, cs, skill, files, rendered, extraFiles, apiClient, hostname, owner, repoName) + } else { + // Interactive with multiple files: show tree, then file picker + renderInteractive(opts, cs, skill, files, rendered, extraFiles, apiClient, hostname, owner, repoName) } - // Interactive with multiple files: show tree, then file picker - return renderInteractive(opts, cs, skill, files, rendered, extraFiles, apiClient, hostname, owner, repoName) + dims := map[string]string{ + "skill_host_type": ghinstance.CategorizeHost(opts.repo.RepoHost()), + } + select { + case r := <-visCh: + if r.err == nil { + dims["repo_visibility"] = string(r.vis) + if r.vis == discovery.RepoVisibilityPublic { + dims["skill_owner"] = opts.repo.RepoOwner() + dims["skill_repo"] = opts.repo.RepoName() + dims["skill_name"] = skill.DisplayName() + } + } else { + dims["repo_visibility"] = "unknown" + } + case <-time.After(visibilityWaitTimeout): + dims["repo_visibility"] = "unknown" + } + opts.Telemetry.Record(ghtelemetry.Event{ + Type: "skill_preview", + Dimensions: dims, + }) + + return nil } +// visibilityWaitTimeout is how long to wait at telemetry-emit time for +// the in-flight repo visibility fetch before giving up and emitting +// repo_visibility="unknown". By this point the command has already done +// several serial API calls and rendering work, so the fetch has almost +// always completed; this budget is a short safety net for the case +// where that single REST call has stalled. +const visibilityWaitTimeout = 200 * time.Millisecond + // renderAllFiles dumps the tree, SKILL.md, and all extra files through the pager. func renderAllFiles(opts *PreviewOptions, cs *iostreams.ColorScheme, skill discovery.Skill, files []discovery.SkillFile, rendered string, extraFiles []discovery.SkillFile, - apiClient *api.Client, hostname, owner, repo string) error { + apiClient *api.Client, hostname, owner, repo string) { opts.IO.DetectTerminalTheme() if err := opts.IO.StartPager(); err != nil { @@ -232,14 +283,12 @@ func renderAllFiles(opts *PreviewOptions, cs *iostreams.ColorScheme, skill disco fmt.Fprintln(out) } } - - return nil } // renderInteractive shows the file tree, then a picker to browse individual files. func renderInteractive(opts *PreviewOptions, cs *iostreams.ColorScheme, skill discovery.Skill, files []discovery.SkillFile, renderedSkillMD string, extraFiles []discovery.SkillFile, - apiClient *api.Client, hostname, owner, repo string) error { + apiClient *api.Client, hostname, owner, repo string) { // Show the file tree to stderr so it persists above the prompt fmt.Fprintf(opts.IO.ErrOut, "\n%s\n", cs.Bold(skill.DisplayName()+"/")) @@ -265,7 +314,7 @@ func renderInteractive(opts *PreviewOptions, cs *iostreams.ColorScheme, skill di idx, err := opts.Prompter.Select("View a file (Esc to exit):", "", choices) if err != nil { - return nil //nolint:nilerr // Prompter returns error on Esc/Ctrl-C; treat as graceful exit + return // Prompter returns error on Esc/Ctrl-C; treat as graceful exit } var content string diff --git a/pkg/cmd/skills/preview/preview_test.go b/pkg/cmd/skills/preview/preview_test.go index 474ce88b5..a5d5554ff 100644 --- a/pkg/cmd/skills/preview/preview_test.go +++ b/pkg/cmd/skills/preview/preview_test.go @@ -11,6 +11,7 @@ import ( "github.com/MakeNowJust/heredoc" "github.com/cli/cli/v2/internal/ghrepo" "github.com/cli/cli/v2/internal/prompter" + "github.com/cli/cli/v2/internal/telemetry" "github.com/cli/cli/v2/pkg/cmdutil" "github.com/cli/cli/v2/pkg/httpmock" "github.com/cli/cli/v2/pkg/iostreams" @@ -74,7 +75,7 @@ func TestNewCmdPreview(t *testing.T) { } var gotOpts *PreviewOptions - cmd := NewCmdPreview(f, func(opts *PreviewOptions) error { + cmd := NewCmdPreview(f, &telemetry.NoOpService{}, func(opts *PreviewOptions) error { gotOpts = opts return nil }) @@ -332,6 +333,7 @@ func TestPreviewRun(t *testing.T) { tt.opts.IO = ios tt.opts.Prompter = &prompter.PrompterMock{} + tt.opts.Telemetry = &telemetry.NoOpService{} err := previewRun(tt.opts) @@ -354,6 +356,7 @@ func TestPreviewRun_UnsupportedHost(t *testing.T) { IO: ios, HttpClient: func() (*http.Client, error) { return &http.Client{}, nil }, repo: ghrepo.NewWithHost("github", "awesome-copilot", "acme.ghes.com"), + Telemetry: &telemetry.NoOpService{}, }) require.ErrorContains(t, err, "supports only github.com") } @@ -415,6 +418,7 @@ func TestPreviewRun_Interactive(t *testing.T) { HttpClient: func() (*http.Client, error) { return &http.Client{Transport: reg}, nil }, Prompter: pm, repo: ghrepo.New("owner", "repo"), + Telemetry: &telemetry.NoOpService{}, } err := previewRun(opts) @@ -510,6 +514,7 @@ func TestPreviewRun_ShowsFileTree(t *testing.T) { Prompter: pm, repo: ghrepo.New("owner", "repo"), SkillName: "my-skill", + Telemetry: &telemetry.NoOpService{}, } err := previewRun(opts) @@ -593,6 +598,7 @@ func TestPreviewRun_ShowsFileTree(t *testing.T) { renderCalls++ return fmt.Sprintf("rendered:%s", filePath) }, + Telemetry: &telemetry.NoOpService{}, } err := previewRun(opts) @@ -618,6 +624,7 @@ func TestPreviewRun_ShowsFileTree(t *testing.T) { Prompter: &prompter.PrompterMock{}, repo: ghrepo.New("owner", "repo"), SkillName: "my-skill", + Telemetry: &telemetry.NoOpService{}, } err := previewRun(opts) @@ -724,6 +731,7 @@ func TestPreviewRun_RenderLimits(t *testing.T) { Prompter: &prompter.PrompterMock{}, repo: ghrepo.New("monalisa", "skills-repo"), SkillName: "my-skill", + Telemetry: &telemetry.NoOpService{}, } err := previewRun(opts) @@ -761,6 +769,7 @@ func TestPreviewRun_RenderLimits(t *testing.T) { Prompter: &prompter.PrompterMock{}, repo: ghrepo.New("monalisa", "skills-repo"), SkillName: "my-skill", + Telemetry: &telemetry.NoOpService{}, } err := previewRun(opts) @@ -793,6 +802,7 @@ func TestPreviewRun_RenderLimits(t *testing.T) { Prompter: &prompter.PrompterMock{}, repo: ghrepo.New("monalisa", "skills-repo"), SkillName: "my-skill", + Telemetry: &telemetry.NoOpService{}, } err := previewRun(opts) @@ -802,3 +812,214 @@ func TestPreviewRun_RenderLimits(t *testing.T) { assert.Contains(t, out, "could not fetch file") }) } + +func TestPreviewRun_InteractiveTelemetryCapturesSelectedSkillName(t *testing.T) { + skillContent := "# Selected Skill\n\nContent here." + encodedContent := base64.StdEncoding.EncodeToString([]byte(skillContent)) + + reg := &httpmock.Registry{} + defer reg.Verify(t) + reg.Register( + httpmock.REST("GET", "repos/owner/repo/releases/latest"), + httpmock.StringResponse(`{"tag_name": "v1.0.0"}`), + ) + reg.Register( + httpmock.REST("GET", "repos/owner/repo/git/ref/tags/v1.0.0"), + httpmock.StringResponse(`{"object": {"sha": "abc123", "type": "commit"}}`), + ) + reg.Register( + httpmock.REST("GET", "repos/owner/repo/git/trees/abc123"), + httpmock.StringResponse(`{ + "sha": "abc123", + "truncated": false, + "tree": [ + {"path": "skills/alpha", "type": "tree", "sha": "tree-a"}, + {"path": "skills/alpha/SKILL.md", "type": "blob", "sha": "blob-a"}, + {"path": "skills/beta", "type": "tree", "sha": "tree-b"}, + {"path": "skills/beta/SKILL.md", "type": "blob", "sha": "blob-b"} + ] + }`), + ) + reg.Register( + httpmock.REST("GET", "repos/owner/repo/git/trees/tree-b"), + httpmock.StringResponse(`{ + "tree": [ + {"path": "SKILL.md", "type": "blob", "sha": "blob-b", "size": 40} + ] + }`), + ) + reg.Register( + httpmock.REST("GET", "repos/owner/repo/git/blobs/blob-b"), + httpmock.StringResponse(`{"sha": "blob-b", "content": "`+encodedContent+`", "encoding": "base64"}`), + ) + reg.Register( + httpmock.REST("GET", "repos/owner/repo"), + httpmock.JSONResponse(map[string]interface{}{ + "visibility": "public", + }), + ) + + ios, _, _, _ := iostreams.Test() + ios.SetStdoutTTY(true) + ios.SetStdinTTY(true) + + pm := &prompter.PrompterMock{ + SelectFunc: func(prompt string, defaultValue string, options []string) (int, error) { + return 1, nil // select "beta" + }, + } + + recorder := &telemetry.EventRecorderSpy{} + + opts := &PreviewOptions{ + IO: ios, + HttpClient: func() (*http.Client, error) { return &http.Client{Transport: reg}, nil }, + Prompter: pm, + Telemetry: recorder, + repo: ghrepo.New("owner", "repo"), + // SkillName intentionally left empty to simulate interactive selection + } + + err := previewRun(opts) + require.NoError(t, err) + + // Verify the telemetry event captured the interactively-selected skill name, not empty string + require.Len(t, recorder.Events, 1) + event := recorder.Events[0] + assert.Equal(t, "skill_preview", event.Type) + assert.Equal(t, "beta", event.Dimensions["skill_name"], "telemetry should capture the selected skill name, not the empty opts.SkillName") +} + +func TestPreviewRun_TelemetryVisibility(t *testing.T) { + skillContent := heredoc.Doc(` + --- + name: my-skill + description: test + --- + # My Skill + Body. + `) + encodedContent := base64.StdEncoding.EncodeToString([]byte(skillContent)) + + tests := []struct { + name string + visibility string + visibilityErr bool + wantSkillNames string + }{ + { + name: "public repo includes skill names", + visibility: "public", + wantSkillNames: "my-skill", + }, + { + name: "private repo excludes skill names", + visibility: "private", + }, + { + name: "internal repo excludes skill names", + visibility: "internal", + }, + { + name: "API error omits visibility and skill names", + visibilityErr: true, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + reg := &httpmock.Registry{} + defer reg.Verify(t) + + reg.Register( + httpmock.REST("GET", "repos/owner/repo/releases/latest"), + httpmock.StringResponse(`{"tag_name": "v1.0.0"}`), + ) + reg.Register( + httpmock.REST("GET", "repos/owner/repo/git/ref/tags/v1.0.0"), + httpmock.StringResponse(`{"object": {"sha": "abc123", "type": "commit"}}`), + ) + reg.Register( + httpmock.REST("GET", "repos/owner/repo/git/trees/abc123"), + httpmock.StringResponse(`{ + "sha": "abc123", + "truncated": false, + "tree": [ + {"path": "skills/my-skill", "type": "tree", "sha": "treeSHA"}, + {"path": "skills/my-skill/SKILL.md", "type": "blob", "sha": "blobSKILL"} + ] + }`), + ) + reg.Register( + httpmock.REST("GET", "repos/owner/repo/git/trees/treeSHA"), + httpmock.StringResponse(`{ + "tree": [ + {"path": "SKILL.md", "type": "blob", "sha": "blobSKILL", "size": 50} + ] + }`), + ) + reg.Register( + httpmock.REST("GET", "repos/owner/repo/git/blobs/blobSKILL"), + httpmock.StringResponse(`{"sha": "blobSKILL", "content": "`+encodedContent+`", "encoding": "base64"}`), + ) + if tt.visibilityErr { + reg.Register( + httpmock.REST("GET", "repos/owner/repo"), + httpmock.StatusStringResponse(500, "server error"), + ) + } else { + reg.Register( + httpmock.REST("GET", "repos/owner/repo"), + httpmock.JSONResponse(map[string]interface{}{ + "visibility": tt.visibility, + }), + ) + } + + ios, _, _, _ := iostreams.Test() + ios.SetStdoutTTY(false) + ios.SetStdinTTY(false) + + recorder := &telemetry.EventRecorderSpy{} + + opts := &PreviewOptions{ + IO: ios, + HttpClient: func() (*http.Client, error) { return &http.Client{Transport: reg}, nil }, + Prompter: &prompter.PrompterMock{}, + Telemetry: recorder, + repo: ghrepo.New("owner", "repo"), + SkillName: "my-skill", + } + + err := previewRun(opts) + require.NoError(t, err) + + require.Len(t, recorder.Events, 1) + event := recorder.Events[0] + assert.Equal(t, "skill_preview", event.Type) + + // skill_host_type is always recorded (categorized, no raw hostname for enterprise/tenancy). + assert.Equal(t, "github.com", event.Dimensions["skill_host_type"]) + + if tt.visibilityErr { + assert.Equal(t, "unknown", event.Dimensions["repo_visibility"], + "visibility fetch errors should emit repo_visibility=\"unknown\" so the fallback is distinguishable from a successful fetch") + } else { + assert.Equal(t, tt.visibility, event.Dimensions["repo_visibility"]) + } + + // Owner, repo, and skill name are only included when the repo + // is public; for private/internal/unknown they are omitted to + // avoid leaking identifiers of non-public repositories. + if tt.wantSkillNames != "" { + assert.Equal(t, "owner", event.Dimensions["skill_owner"]) + assert.Equal(t, "repo", event.Dimensions["skill_repo"]) + assert.Equal(t, tt.wantSkillNames, event.Dimensions["skill_name"]) + } else { + assert.Empty(t, event.Dimensions["skill_owner"]) + assert.Empty(t, event.Dimensions["skill_repo"]) + assert.Empty(t, event.Dimensions["skill_name"]) + } + }) + } +} diff --git a/pkg/cmd/skills/search/search.go b/pkg/cmd/skills/search/search.go index 5bcc15bda..5f510aae2 100644 --- a/pkg/cmd/skills/search/search.go +++ b/pkg/cmd/skills/search/search.go @@ -15,6 +15,7 @@ import ( "github.com/MakeNowJust/heredoc" "github.com/cli/cli/v2/api" "github.com/cli/cli/v2/internal/gh" + "github.com/cli/cli/v2/internal/gh/ghtelemetry" "github.com/cli/cli/v2/internal/prompter" "github.com/cli/cli/v2/internal/skills/discovery" "github.com/cli/cli/v2/internal/skills/frontmatter" @@ -48,6 +49,7 @@ var SkillSearchFields = []string{ type SearchOptions struct { IO *iostreams.IOStreams + Telemetry ghtelemetry.EventRecorder HttpClient func() (*http.Client, error) Config func() (gh.Config, error) Prompter prompter.Prompter @@ -62,9 +64,10 @@ type SearchOptions struct { } // NewCmdSearch creates the "skills search" command. -func NewCmdSearch(f *cmdutil.Factory, runF func(*SearchOptions) error) *cobra.Command { +func NewCmdSearch(f *cmdutil.Factory, telemetry ghtelemetry.CommandRecorder, runF func(*SearchOptions) error) *cobra.Command { opts := &SearchOptions{ IO: f.IOStreams, + Telemetry: telemetry, HttpClient: f.HttpClient, Config: f.Config, Prompter: f.Prompter, @@ -552,6 +555,13 @@ func promptInstall(opts *SearchOptions, skills []skillResult) error { return nil } + opts.Telemetry.Record(ghtelemetry.Event{ + Type: "skill_search_install", + Measures: ghtelemetry.Measures{ + "install_count": int64(len(indices)), + }, + }) + // Prompt for target agent host (once for all selected skills) hostNames := registry.AgentNames() hostIdx, err := opts.Prompter.Select("Select target agent:", "", hostNames) diff --git a/pkg/cmd/skills/search/search_test.go b/pkg/cmd/skills/search/search_test.go index bdfe3ba19..763ca2124 100644 --- a/pkg/cmd/skills/search/search_test.go +++ b/pkg/cmd/skills/search/search_test.go @@ -8,6 +8,8 @@ import ( "github.com/cli/cli/v2/internal/config" "github.com/cli/cli/v2/internal/gh" + "github.com/cli/cli/v2/internal/prompter" + "github.com/cli/cli/v2/internal/telemetry" "github.com/cli/cli/v2/pkg/cmdutil" "github.com/cli/cli/v2/pkg/httpmock" "github.com/cli/cli/v2/pkg/iostreams" @@ -102,7 +104,7 @@ func TestNewCmdSearch(t *testing.T) { t.Run(tt.name, func(t *testing.T) { f := &cmdutil.Factory{} var gotOpts *SearchOptions - cmd := NewCmdSearch(f, func(opts *SearchOptions) error { + cmd := NewCmdSearch(f, &telemetry.NoOpService{}, func(opts *SearchOptions) error { gotOpts = opts return nil }) @@ -372,6 +374,7 @@ func TestSearchRun(t *testing.T) { ios.SetStdoutTTY(tt.tty) ios.SetStderrTTY(tt.tty) tt.opts.IO = ios + tt.opts.Telemetry = &telemetry.NoOpService{} defer reg.Verify(t) err := searchRun(tt.opts) @@ -576,3 +579,75 @@ func TestDeduplicateByName_Namespaced(t *testing.T) { assert.NotEqual(t, "org/repo6", s.Repo) } } + +// TestSearchRun_TelemetryRecordsInstallFromResults verifies that when a +// user searches, picks one or more results interactively, and proceeds to +// install them, the search command records a telemetry event capturing +// that the search led to an install attempt. This is the key signal for +// measuring the value of search results: of the searches that ran, how +// many converted to an install? +func TestSearchRun_TelemetryRecordsInstallFromResults(t *testing.T) { + codeResponse := `{"total_count": 1, "incomplete_results": false, "items": [ + {"name": "SKILL.md", "path": "skills/terraform/SKILL.md", "sha": "abc123", + "repository": {"full_name": "org/repo"}} + ]}` + + reg := &httpmock.Registry{} + defer reg.Verify(t) + // Keyword search fires path + owner + primary (3 requests). + for range 3 { + reg.Register( + httpmock.REST("GET", "search/code"), + httpmock.StringResponse(codeResponse), + ) + } + + ios, _, _, _ := iostreams.Test() + ios.SetStdoutTTY(true) + ios.SetStderrTTY(true) + ios.SetStdinTTY(true) + + pm := &prompter.PrompterMock{ + MultiSelectFunc: func(prompt string, defaults []string, options []string) ([]int, error) { + // Select the single result. + return []int{0}, nil + }, + SelectFunc: func(prompt, defaultValue string, options []string) (int, error) { + // First Select: target agent (0). Second Select: scope (0). + return 0, nil + }, + } + + recorder := &telemetry.EventRecorderSpy{} + + err := searchRun(&SearchOptions{ + IO: ios, + HttpClient: func() (*http.Client, error) { return &http.Client{Transport: reg}, nil }, + Config: func() (gh.Config, error) { return config.NewBlankConfig(), nil }, + Prompter: pm, + Telemetry: recorder, + ExecutablePath: "/nonexistent/gh", // install subprocess will fail; failures are logged, not fatal. + Query: "terraform", + Page: 1, + Limit: defaultLimit, + }) + require.NoError(t, err) + + // The search command no longer records a separate skill_search event; + // only the follow-up skill_search_install event fires when the user + // proceeds to install from the results. + require.Len(t, recorder.Events, 1) + + installEvent := recorder.Events[0] + assert.Equal(t, "skill_search_install", installEvent.Type, + "an install triggered from search results should be recorded as a distinct event") + assert.Equal(t, int64(1), installEvent.Measures["install_count"], + "install_count captures how many results the user chose to install") + // The skill_search_install event must not carry the query or owner: + // these were intentionally removed so that installs from search are + // not linked back to the search terms at the telemetry layer. + assert.Empty(t, installEvent.Dimensions["query"], + "skill_search_install must not record the search query") + assert.Empty(t, installEvent.Dimensions["owner"], + "skill_search_install must not record the search owner filter") +} diff --git a/pkg/cmd/skills/skills.go b/pkg/cmd/skills/skills.go index 1dadd3b1f..05a87c386 100644 --- a/pkg/cmd/skills/skills.go +++ b/pkg/cmd/skills/skills.go @@ -2,6 +2,7 @@ package skills import ( "github.com/MakeNowJust/heredoc" + "github.com/cli/cli/v2/internal/gh/ghtelemetry" "github.com/cli/cli/v2/pkg/cmd/skills/install" "github.com/cli/cli/v2/pkg/cmd/skills/preview" "github.com/cli/cli/v2/pkg/cmd/skills/publish" @@ -12,7 +13,7 @@ import ( ) // NewCmdSkills returns the top-level "skill" command. -func NewCmdSkills(f *cmdutil.Factory) *cobra.Command { +func NewCmdSkills(f *cmdutil.Factory, telemetry ghtelemetry.CommandRecorder) *cobra.Command { cmd := &cobra.Command{ Use: "skill ", Short: "Install and manage agent skills (preview)", @@ -40,12 +41,16 @@ func NewCmdSkills(f *cmdutil.Factory) *cobra.Command { # Validate skills for publishing $ gh skill publish --dry-run `), + PersistentPreRunE: func(cmd *cobra.Command, args []string) error { + telemetry.SetSampleRate(ghtelemetry.SAMPLE_ALL) + return nil + }, } - cmd.AddCommand(install.NewCmdInstall(f, nil)) - cmd.AddCommand(preview.NewCmdPreview(f, nil)) + cmd.AddCommand(install.NewCmdInstall(f, telemetry, nil)) + cmd.AddCommand(preview.NewCmdPreview(f, telemetry, nil)) cmd.AddCommand(publish.NewCmdPublish(f, nil)) - cmd.AddCommand(search.NewCmdSearch(f, nil)) + cmd.AddCommand(search.NewCmdSearch(f, telemetry, nil)) cmd.AddCommand(update.NewCmdUpdate(f, nil)) return cmd diff --git a/pkg/cmd/skills/skills_test.go b/pkg/cmd/skills/skills_test.go new file mode 100644 index 000000000..eb8bb465c --- /dev/null +++ b/pkg/cmd/skills/skills_test.go @@ -0,0 +1,19 @@ +package skills_test + +import ( + "testing" + + "github.com/cli/cli/v2/internal/gh/ghtelemetry" + "github.com/cli/cli/v2/internal/telemetry" + "github.com/cli/cli/v2/pkg/cmd/skills" + "github.com/cli/cli/v2/pkg/cmdutil" + "github.com/stretchr/testify/require" +) + +func TestSkillCommandsAreSampledAt100(t *testing.T) { + spy := &telemetry.CommandRecorderSpy{} + factory := &cmdutil.Factory{} + cmd := skills.NewCmdSkills(factory, spy) + cmd.PersistentPreRunE(nil, []string{}) + require.Equal(t, ghtelemetry.SAMPLE_ALL, spy.LastSampleRate) +}