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 | 69 ++++-- 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, 895 insertions(+), 48 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 c365cb83389..0311a0db280 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 be1be5244d7..af1d0bbbe2c 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 cb32fa7e26e..786204951e1 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 30c044f78cf..48409c2354d 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 5e8c7744208..e16936b0d1b 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 7abfd83acaf..dfea6dd946b 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 1b7e0146d04..c4f447780a9 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 93de67faac5..2d6c1ee7256 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" +) -type releaseResponse struct { - TagName string `json:"tag_name"` +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 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 f9abc593cf8..8929e17871b 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 1dc45ab2692..4eb22e898a5 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 7df9d2986ea..9f4fa6f5bdc 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 1b6a7fd8fb4..d4a05440e7b 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 4812275247f..0560625f7fc 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 4bdfdd416b4..e9f1e0442ce 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 474ce88b5aa..a5d5554ffe9 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 5bcc15bda3a..5f510aae2d2 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 bdfe3ba1913..763ca2124e1 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 1dadd3b1f4e..05a87c38651 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 00000000000..eb8bb465c0e --- /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) +}