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) +}