diff --git a/internal/skills/discovery/discovery_test.go b/internal/skills/discovery/discovery_test.go index 7f74c2998..f9abc593c 100644 --- a/internal/skills/discovery/discovery_test.go +++ b/internal/skills/discovery/discovery_test.go @@ -925,21 +925,21 @@ func TestMatchesSkillPath(t *testing.T) { func TestMatchSkillPath(t *testing.T) { tests := []struct { - testName string + name string path string wantName string wantNamespace string }{ - {testName: "skills convention", path: "skills/code-review/SKILL.md", wantName: "code-review", wantNamespace: ""}, - {testName: "namespaced convention", path: "skills/monalisa/issue-triage/SKILL.md", wantName: "issue-triage", wantNamespace: "monalisa"}, - {testName: "plugins convention", path: "plugins/hubot/skills/pr-summary/SKILL.md", wantName: "pr-summary", wantNamespace: "hubot"}, - {testName: "non-skill file", path: "README.md", wantName: "", wantNamespace: ""}, - {testName: "same name different namespace 1", path: "skills/kynan/commit/SKILL.md", wantName: "commit", wantNamespace: "kynan"}, - {testName: "same name different namespace 2", path: "skills/will/commit/SKILL.md", wantName: "commit", wantNamespace: "will"}, - {testName: "root convention", path: "my-skill/SKILL.md", wantName: "my-skill", wantNamespace: ""}, + {name: "skills convention", path: "skills/code-review/SKILL.md", wantName: "code-review", wantNamespace: ""}, + {name: "namespaced convention", path: "skills/monalisa/issue-triage/SKILL.md", wantName: "issue-triage", wantNamespace: "monalisa"}, + {name: "plugins convention", path: "plugins/hubot/skills/pr-summary/SKILL.md", wantName: "pr-summary", wantNamespace: "hubot"}, + {name: "non-skill file", path: "README.md", wantName: "", wantNamespace: ""}, + {name: "same name different namespace 1", path: "skills/kynan/commit/SKILL.md", wantName: "commit", wantNamespace: "kynan"}, + {name: "same name different namespace 2", path: "skills/will/commit/SKILL.md", wantName: "commit", wantNamespace: "will"}, + {name: "root convention", path: "my-skill/SKILL.md", wantName: "my-skill", wantNamespace: ""}, } for _, tt := range tests { - t.Run(tt.testName, func(t *testing.T) { + t.Run(tt.name, func(t *testing.T) { name, namespace := MatchSkillPath(tt.path) assert.Equal(t, tt.wantName, name) assert.Equal(t, tt.wantNamespace, namespace) diff --git a/pkg/cmd/skills/publish/publish.go b/pkg/cmd/skills/publish/publish.go index 213afeba5..d82781876 100644 --- a/pkg/cmd/skills/publish/publish.go +++ b/pkg/cmd/skills/publish/publish.go @@ -42,8 +42,7 @@ type PublishOptions struct { DryRun bool Tag string - client *api.Client // injectable for tests; nil means use factory - host string // resolved from config in production + host string // resolved from config in production } // publishDiagnostic is a single validation finding. @@ -178,11 +177,10 @@ func publishRun(opts *PublishOptions) error { canPrompt := opts.IO.CanPrompt() - // Use injected client or create one from the factory HttpClient. - // Initialization is deferred until after local validation so that + // Client initialization is deferred until after local validation so that // simple errors (missing skills/, bad SKILL.md, etc.) are reported // without requiring an HTTP client. - client := opts.client + var client *api.Client host := opts.host var diagnostics []publishDiagnostic @@ -343,14 +341,11 @@ func publishRun(opts *PublishOptions) error { hasTopic := false var existingTags []tagEntry if owner != "" && repo != "" { - // Create API client from factory if not already injected (tests inject directly). - if client == nil { - httpClient, err := opts.HttpClient() - if err != nil { - return err - } - client = api.NewClientFromHTTP(httpClient) + httpClient, err := opts.HttpClient() + if err != nil { + return err } + client = api.NewClientFromHTTP(httpClient) if host == "" && repoInfo != nil { host = repoInfo.Repo.RepoHost() @@ -658,10 +653,11 @@ func ensurePushed(opts *PublishOptions, dir, remoteName string) error { } ref := fmt.Sprintf("HEAD:refs/heads/%s", currentBranch) - fmt.Fprintf(opts.IO.ErrOut, "%s Pushing %s to %s\n", cs.SuccessIcon(), currentBranch, remoteName) + fmt.Fprintf(opts.IO.ErrOut, "Pushing %s to %s...\n", currentBranch, remoteName) if err := gitClient.Push(ctx, remoteName, ref); err != nil { return fmt.Errorf("failed to push branch %s: %w", currentBranch, err) } + fmt.Fprintf(opts.IO.ErrOut, "%s Pushed %s to %s\n", cs.SuccessIcon(), currentBranch, remoteName) return nil } @@ -924,7 +920,9 @@ type gitHubRemote struct { } // detectGitHubRemote attempts to detect the GitHub owner/repo from git remotes -// in the given directory. +// in the given directory. Remotes are tried in the order returned by +// gitClient.Remotes (upstream > github > origin > rest), so the first +// GitHub-pointing remote wins. func detectGitHubRemote(gitClient *git.Client, dir string) (*gitHubRemote, error) { if gitClient == nil { return nil, nil @@ -933,26 +931,11 @@ func detectGitHubRemote(gitClient *git.Client, dir string) (*gitHubRemote, error dirClient := gitClient.Copy() dirClient.RepoDir = dir - // Try origin first - if url, err := dirClient.RemoteURL(context.Background(), "origin"); err == nil { - repo, parseErr := parseGitHubURL(url) - if parseErr != nil { - return nil, parseErr - } - if repo != nil { - return &gitHubRemote{Repo: repo, RemoteName: "origin"}, nil - } - } - - // Fall back to any remote that points to GitHub remotes, err := dirClient.Remotes(context.Background()) if err != nil { return nil, nil //nolint:nilerr // failing to list remotes is not an error; it just means no repo detected } for _, r := range remotes { - if r.Name == "origin" { - continue - } if url, err := dirClient.RemoteURL(context.Background(), r.Name); err == nil { repo, parseErr := parseGitHubURL(url) if parseErr != nil { diff --git a/pkg/cmd/skills/publish/publish_test.go b/pkg/cmd/skills/publish/publish_test.go index 8c7205a3d..97795a617 100644 --- a/pkg/cmd/skills/publish/publish_test.go +++ b/pkg/cmd/skills/publish/publish_test.go @@ -10,7 +10,6 @@ import ( "testing" "github.com/MakeNowJust/heredoc" - "github.com/cli/cli/v2/api" "github.com/cli/cli/v2/git" "github.com/cli/cli/v2/internal/prompter" "github.com/cli/cli/v2/pkg/cmdutil" @@ -162,11 +161,11 @@ func TestPublishRun_UnsupportedHost(t *testing.T) { ios, _, _, _ := iostreams.Test() initGitRepo(t, dir, map[string]string{"origin": "https://github.com/monalisa/skills-repo.git"}) err := publishRun(&PublishOptions{ - IO: ios, - Dir: dir, - GitClient: &git.Client{}, - client: api.NewClientFromHTTP(&http.Client{}), - host: "acme.ghes.com", + IO: ios, + Dir: dir, + GitClient: &git.Client{}, + HttpClient: func() (*http.Client, error) { return nil, nil }, + host: "acme.ghes.com", }) require.ErrorContains(t, err, "supports only github.com") } @@ -286,12 +285,12 @@ func TestPublishRun(t *testing.T) { opts: func(ios *iostreams.IOStreams, dir string, reg *httpmock.Registry) *PublishOptions { t.Helper() return &PublishOptions{ - IO: ios, - Dir: dir, - DryRun: true, - GitClient: &git.Client{}, - client: api.NewClientFromHTTP(&http.Client{Transport: reg}), - host: "github.com", + IO: ios, + Dir: dir, + DryRun: true, + GitClient: &git.Client{}, + HttpClient: func() (*http.Client, error) { return &http.Client{Transport: reg}, nil }, + host: "github.com", } }, wantStdout: "1 skill(s) validated successfully", @@ -323,12 +322,12 @@ func TestPublishRun(t *testing.T) { opts: func(ios *iostreams.IOStreams, dir string, reg *httpmock.Registry) *PublishOptions { t.Helper() return &PublishOptions{ - IO: ios, - Dir: dir, - DryRun: true, - GitClient: &git.Client{}, - client: api.NewClientFromHTTP(&http.Client{Transport: reg}), - host: "github.com", + IO: ios, + Dir: dir, + DryRun: true, + GitClient: &git.Client{}, + HttpClient: func() (*http.Client, error) { return &http.Client{Transport: reg}, nil }, + host: "github.com", } }, wantStdout: "1 skill(s) validated successfully", @@ -357,12 +356,12 @@ func TestPublishRun(t *testing.T) { "origin": "https://github.com/monalisa/skills-repo.git", }) return &PublishOptions{ - IO: ios, - Dir: dir, - DryRun: true, - GitClient: &git.Client{}, - client: api.NewClientFromHTTP(&http.Client{Transport: reg}), - host: "github.com", + IO: ios, + Dir: dir, + DryRun: true, + GitClient: &git.Client{}, + HttpClient: func() (*http.Client, error) { return &http.Client{Transport: reg}, nil }, + host: "github.com", } }, wantStdout: "1 skill(s) validated successfully", @@ -416,9 +415,9 @@ func TestPublishRun(t *testing.T) { Prompter: &prompter.PrompterMock{ ConfirmFunc: func(msg string, def bool) (bool, error) { return true, nil }, }, - GitClient: &git.Client{}, - client: api.NewClientFromHTTP(&http.Client{Transport: reg}), - host: "github.com", + GitClient: &git.Client{}, + HttpClient: func() (*http.Client, error) { return &http.Client{Transport: reg}, nil }, + host: "github.com", } }, wantStdout: "Published v1.0.1", @@ -564,11 +563,11 @@ func TestPublishRun(t *testing.T) { "origin": "https://github.com/octocat/secure-repo.git", }) return &PublishOptions{ - IO: ios, - Dir: dir, - GitClient: &git.Client{}, - client: api.NewClientFromHTTP(&http.Client{Transport: reg}), - host: "github.com", + IO: ios, + Dir: dir, + GitClient: &git.Client{}, + HttpClient: func() (*http.Client, error) { return &http.Client{Transport: reg}, nil }, + host: "github.com", } }, wantStdout: "secret scanning is not enabled", @@ -617,11 +616,11 @@ func TestPublishRun(t *testing.T) { "origin": "https://github.com/octocat/tag-repo.git", }) return &PublishOptions{ - IO: ios, - Dir: dir, - GitClient: &git.Client{}, - client: api.NewClientFromHTTP(&http.Client{Transport: reg}), - host: "github.com", + IO: ios, + Dir: dir, + GitClient: &git.Client{}, + HttpClient: func() (*http.Client, error) { return &http.Client{Transport: reg}, nil }, + host: "github.com", } }, wantStdout: "tag protection", @@ -680,12 +679,12 @@ func TestPublishRun(t *testing.T) { "origin": "https://github.com/octocat/code-repo.git", }) return &PublishOptions{ - IO: ios, - Dir: dir, - DryRun: true, - GitClient: &git.Client{}, - client: api.NewClientFromHTTP(&http.Client{Transport: reg}), - host: "github.com", + IO: ios, + Dir: dir, + DryRun: true, + GitClient: &git.Client{}, + HttpClient: func() (*http.Client, error) { return &http.Client{Transport: reg}, nil }, + host: "github.com", } }, wantStderr: "code scanning", @@ -745,12 +744,12 @@ func TestPublishRun(t *testing.T) { "origin": "https://github.com/octocat/dep-repo.git", }) return &PublishOptions{ - IO: ios, - Dir: dir, - DryRun: true, - GitClient: &git.Client{}, - client: api.NewClientFromHTTP(&http.Client{Transport: reg}), - host: "github.com", + IO: ios, + Dir: dir, + DryRun: true, + GitClient: &git.Client{}, + HttpClient: func() (*http.Client, error) { return &http.Client{Transport: reg}, nil }, + host: "github.com", } }, wantStderr: "Dependabot", @@ -895,12 +894,12 @@ func TestPublishRun(t *testing.T) { "upstream": "git@github.com:octocat/repo.git", }) return &PublishOptions{ - IO: ios, - Dir: dir, - DryRun: true, - GitClient: &git.Client{}, - client: api.NewClientFromHTTP(&http.Client{Transport: reg}), - host: "github.com", + IO: ios, + Dir: dir, + DryRun: true, + GitClient: &git.Client{}, + HttpClient: func() (*http.Client, error) { return &http.Client{Transport: reg}, nil }, + host: "github.com", } }, wantStderr: "octocat/repo", @@ -987,9 +986,9 @@ func TestPublishRun(t *testing.T) { Prompter: &prompter.PrompterMock{ ConfirmFunc: func(msg string, def bool) (bool, error) { return true, nil }, }, - GitClient: &git.Client{}, - client: api.NewClientFromHTTP(&http.Client{Transport: reg}), - host: "github.com", + GitClient: &git.Client{}, + HttpClient: func() (*http.Client, error) { return &http.Client{Transport: reg}, nil }, + host: "github.com", } }, wantStdout: "Added \"agent-skills\" topic", @@ -1059,12 +1058,12 @@ func TestPublishRun(t *testing.T) { "origin": "https://github.com/monalisa/skills-repo.git", }) return &PublishOptions{ - IO: ios, - Dir: dir, - Tag: "v2.3.5", - GitClient: &git.Client{}, - client: api.NewClientFromHTTP(&http.Client{Transport: reg}), - host: "github.com", + IO: ios, + Dir: dir, + Tag: "v2.3.5", + GitClient: &git.Client{}, + HttpClient: func() (*http.Client, error) { return &http.Client{Transport: reg}, nil }, + host: "github.com", } }, wantStdout: "Published v2.3.5", @@ -1091,12 +1090,12 @@ func TestPublishRun(t *testing.T) { "origin": "https://github.com/monalisa/skills-repo.git", }) return &PublishOptions{ - IO: ios, - Dir: dir, - Tag: "v1.0.0", // same as stubAllSecureRemote's existing tag - GitClient: &git.Client{}, - client: api.NewClientFromHTTP(&http.Client{Transport: reg}), - host: "github.com", + IO: ios, + Dir: dir, + Tag: "v1.0.0", // same as stubAllSecureRemote's existing tag + GitClient: &git.Client{}, + HttpClient: func() (*http.Client, error) { return &http.Client{Transport: reg}, nil }, + host: "github.com", } }, wantErr: "tag v1.0.0 already exists", @@ -1124,11 +1123,11 @@ func TestPublishRun(t *testing.T) { "origin": "https://github.com/monalisa/skills-repo.git", }) return &PublishOptions{ - IO: ios, - Dir: dir, - GitClient: &git.Client{}, - client: api.NewClientFromHTTP(&http.Client{Transport: reg}), - host: "github.com", + IO: ios, + Dir: dir, + GitClient: &git.Client{}, + HttpClient: func() (*http.Client, error) { return &http.Client{Transport: reg}, nil }, + host: "github.com", } }, wantStdout: "ok", @@ -1237,9 +1236,9 @@ func TestPublishRun(t *testing.T) { return "v1.0.0", nil // accept suggested tag }, }, - GitClient: &git.Client{}, - client: api.NewClientFromHTTP(&http.Client{Transport: reg}), - host: "github.com", + GitClient: &git.Client{}, + HttpClient: func() (*http.Client, error) { return &http.Client{Transport: reg}, nil }, + host: "github.com", } }, wantStdout: "Published v1.0.0", @@ -1294,9 +1293,9 @@ func TestPublishRun(t *testing.T) { return "beta-1", nil }, }, - GitClient: &git.Client{}, - client: api.NewClientFromHTTP(&http.Client{Transport: reg}), - host: "github.com", + GitClient: &git.Client{}, + HttpClient: func() (*http.Client, error) { return &http.Client{Transport: reg}, nil }, + host: "github.com", } }, wantStdout: "Published beta-1", @@ -1350,9 +1349,9 @@ func TestPublishRun(t *testing.T) { return "v1.0.1", nil }, }, - GitClient: &git.Client{}, - client: api.NewClientFromHTTP(&http.Client{Transport: reg}), - host: "github.com", + GitClient: &git.Client{}, + HttpClient: func() (*http.Client, error) { return &http.Client{Transport: reg}, nil }, + host: "github.com", } }, wantErr: "CancelError", @@ -1414,9 +1413,9 @@ func TestPublishRun(t *testing.T) { return "v1.0.1", nil }, }, - GitClient: &git.Client{}, - client: api.NewClientFromHTTP(&http.Client{Transport: reg}), - host: "github.com", + GitClient: &git.Client{}, + HttpClient: func() (*http.Client, error) { return &http.Client{Transport: reg}, nil }, + host: "github.com", } }, wantStdout: "Enabled immutable releases", @@ -1528,12 +1527,12 @@ func TestPublishRun_DirArgUsesTargetRemote(t *testing.T) { stubAllSecureRemote(reg, "monalisa", "target-repo") err := publishRun(&PublishOptions{ - IO: ios, - Dir: targetRepo, - DryRun: true, - GitClient: &git.Client{RepoDir: cwdRepo}, - client: api.NewClientFromHTTP(&http.Client{Transport: reg}), - host: "github.com", + IO: ios, + Dir: targetRepo, + DryRun: true, + GitClient: &git.Client{RepoDir: cwdRepo}, + HttpClient: func() (*http.Client, error) { return &http.Client{Transport: reg}, nil }, + host: "github.com", }) require.NoError(t, err) diff --git a/pkg/cmd/skills/search/search.go b/pkg/cmd/skills/search/search.go index f7d4975a7..05511484e 100644 --- a/pkg/cmd/skills/search/search.go +++ b/pkg/cmd/skills/search/search.go @@ -772,7 +772,14 @@ func fetchPrimaryPages(client *api.Client, host, query string, displayPage, disp // deduplicateResults extracts unique (repo, namespace, skill name) triples from code search hits. func deduplicateResults(items []codeSearchItem) []skillResult { - seen := make(map[string]struct{}) + // skillResultKey is a typed map key that deduplicates by (repo, namespace, + // skill name). All fields are lowercased for case-insensitive comparison. + type skillResultKey struct { + repo string + namespace string + skillName string + } + seen := make(map[skillResultKey]struct{}) var results []skillResult for _, item := range items { @@ -780,7 +787,11 @@ func deduplicateResults(items []codeSearchItem) []skillResult { if skillName == "" { continue } - key := item.Repository.FullName + "/" + namespace + "/" + skillName + key := skillResultKey{ + repo: strings.ToLower(item.Repository.FullName), + namespace: strings.ToLower(namespace), + skillName: strings.ToLower(skillName), + } if _, ok := seen[key]; ok { continue }