diff --git a/internal/skills/frontmatter/frontmatter.go b/internal/skills/frontmatter/frontmatter.go index 034068884..87ad067a0 100644 --- a/internal/skills/frontmatter/frontmatter.go +++ b/internal/skills/frontmatter/frontmatter.go @@ -5,6 +5,7 @@ import ( "fmt" "strings" + "github.com/cli/cli/v2/internal/skills/source" "gopkg.in/yaml.v3" ) @@ -66,7 +67,7 @@ func Parse(content string) (*ParseResult, error) { // collisions with other tools' metadata. // pinnedRef is the user's explicit --pin value; empty string means unpinned. // skillPath is the skill's source path in the repo (e.g. "skills/author/my-skill"). -func InjectGitHubMetadata(content string, owner, repo, ref, sha, treeSHA, pinnedRef, skillPath string) (string, error) { +func InjectGitHubMetadata(content string, host, owner, repo, ref, treeSHA, pinnedRef, skillPath string) (string, error) { result, err := Parse(content) if err != nil { return "", err @@ -80,10 +81,10 @@ func InjectGitHubMetadata(content string, owner, repo, ref, sha, treeSHA, pinned if meta == nil { meta = make(map[string]interface{}) } - meta["github-owner"] = owner - meta["github-repo"] = repo + delete(meta, "github-owner") + meta["github-repo"] = source.BuildRepoURL(host, owner, repo) meta["github-ref"] = ref - meta["github-sha"] = sha + delete(meta, "github-sha") meta["github-tree-sha"] = treeSHA meta["github-path"] = skillPath if pinnedRef != "" { diff --git a/internal/skills/frontmatter/frontmatter_test.go b/internal/skills/frontmatter/frontmatter_test.go index 51fe09133..229eadd18 100644 --- a/internal/skills/frontmatter/frontmatter_test.go +++ b/internal/skills/frontmatter/frontmatter_test.go @@ -67,10 +67,10 @@ func TestInjectGitHubMetadata(t *testing.T) { tests := []struct { name string content string + host string owner string repo string ref string - sha string treeSHA string pinnedRef string skillPath string @@ -86,23 +86,23 @@ func TestInjectGitHubMetadata(t *testing.T) { --- # Body `), + host: "github.com", owner: "monalisa", repo: "octocat-skills", ref: "v1.0.0", - sha: "abc123", treeSHA: "tree456", pinnedRef: "", skillPath: "skills/my-skill", wantContains: []string{ - "github-owner: monalisa", - "github-repo: octocat-skills", + "github-repo: https://github.com/monalisa/octocat-skills", "github-ref: v1.0.0", - "github-sha: abc123", "github-tree-sha: tree456", "github-path: skills/my-skill", "# Body", }, wantNotContain: []string{ + "github-owner", + "github-sha", "github-pinned", }, }, @@ -114,10 +114,10 @@ func TestInjectGitHubMetadata(t *testing.T) { --- # Body `), + host: "github.com", owner: "monalisa", repo: "octocat-skills", ref: "v1.0.0", - sha: "abc", treeSHA: "tree", pinnedRef: "v1.0.0", skillPath: "skills/my-skill", @@ -128,24 +128,24 @@ func TestInjectGitHubMetadata(t *testing.T) { { name: "injects metadata into content with no frontmatter", content: "# Body only\n", + host: "github.com", owner: "monalisa", repo: "octocat-skills", ref: "v1.0.0", - sha: "abc123", treeSHA: "tree456", pinnedRef: "", skillPath: "skills/my-skill", wantContains: []string{ - "github-owner: monalisa", - "github-repo: octocat-skills", + "github-repo: https://github.com/monalisa/octocat-skills", "# Body only", }, + wantNotContain: []string{"github-owner", "github-sha"}, }, } for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { - got, err := InjectGitHubMetadata(tt.content, tt.owner, tt.repo, tt.ref, tt.sha, tt.treeSHA, tt.pinnedRef, tt.skillPath) + got, err := InjectGitHubMetadata(tt.content, tt.host, tt.owner, tt.repo, tt.ref, tt.treeSHA, tt.pinnedRef, tt.skillPath) require.NoError(t, err) for _, s := range tt.wantContains { assert.Contains(t, got, s) diff --git a/internal/skills/installer/installer.go b/internal/skills/installer/installer.go index ce5370004..5fdf99ce0 100644 --- a/internal/skills/installer/installer.go +++ b/internal/skills/installer/installer.go @@ -282,7 +282,7 @@ func installSkill(opts *Options, skill discovery.Skill, baseDir string) error { } if filepath.Base(relPath) == "SKILL.md" { - content, err = frontmatter.InjectGitHubMetadata(content, opts.Owner, opts.Repo, opts.Ref, file.SHA, skill.TreeSHA, opts.PinnedRef, skill.Path) + content, err = frontmatter.InjectGitHubMetadata(content, opts.Host, opts.Owner, opts.Repo, opts.Ref, skill.TreeSHA, opts.PinnedRef, skill.Path) if err != nil { return fmt.Errorf("could not inject metadata: %w", err) } diff --git a/internal/skills/installer/installer_test.go b/internal/skills/installer/installer_test.go index ea9c619c2..85c8bcf18 100644 --- a/internal/skills/installer/installer_test.go +++ b/internal/skills/installer/installer_test.go @@ -248,8 +248,8 @@ func TestInstallSkill(t *testing.T) { t.Helper() content, err := os.ReadFile(filepath.Join(destDir, "pr-summary", "SKILL.md")) require.NoError(t, err) - assert.Contains(t, string(content), "github-owner: monalisa") - assert.Contains(t, string(content), "github-repo: octocat-skills") + assert.NotContains(t, string(content), "github-owner:") + assert.Contains(t, string(content), "github-repo: https://github.com/monalisa/octocat-skills") }, }, { diff --git a/internal/skills/source/source.go b/internal/skills/source/source.go new file mode 100644 index 000000000..5e8f52888 --- /dev/null +++ b/internal/skills/source/source.go @@ -0,0 +1,66 @@ +package source + +import ( + "fmt" + "strings" + + "github.com/cli/cli/v2/internal/ghrepo" +) + +const SupportedHost = "github.com" + +// BuildRepoURL returns the canonical repository URL stored in skill metadata. +func BuildRepoURL(host, owner, repo string) string { + return ghrepo.GenerateRepoURL(ghrepo.NewWithHost(owner, repo, host), "") +} + +// ParseRepoURL parses a repository URL stored in skill metadata. +func ParseRepoURL(raw string) (ghrepo.Interface, error) { + raw = strings.TrimSpace(raw) + if raw == "" { + return nil, fmt.Errorf("repository URL is empty") + } + + repo, err := ghrepo.FromFullName(raw) + if err != nil { + return nil, fmt.Errorf("invalid repository URL %q: %w", raw, err) + } + + return repo, nil +} + +// ParseMetadataRepo extracts repository information from skill metadata. +func ParseMetadataRepo(meta map[string]interface{}) (ghrepo.Interface, bool, error) { + if meta == nil { + return nil, false, nil + } + + repoValue, _ := meta["github-repo"].(string) + if repoValue == "" { + return nil, false, nil + } + + repo, err := ParseRepoURL(repoValue) + if err != nil { + return nil, true, err + } + + return repo, true, nil +} + +// ValidateSupportedHost rejects hosts that are not supported in public preview. +func ValidateSupportedHost(host string) error { + host = normalizeHost(host) + if host == "" { + return fmt.Errorf("could not determine repository host") + } + if host != SupportedHost { + return fmt.Errorf("GitHub Skills currently supports only %s as a host; got %s", SupportedHost, host) + } + return nil +} + +func normalizeHost(host string) string { + host = strings.TrimSpace(strings.ToLower(host)) + return strings.TrimPrefix(host, "www.") +} diff --git a/internal/skills/source/source_test.go b/internal/skills/source/source_test.go new file mode 100644 index 000000000..f797591b4 --- /dev/null +++ b/internal/skills/source/source_test.go @@ -0,0 +1,76 @@ +package source + +import ( + "testing" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +func TestBuildRepoURL(t *testing.T) { + assert.Equal(t, "https://github.com/monalisa/octocat-skills", BuildRepoURL("github.com", "monalisa", "octocat-skills")) +} + +func TestParseMetadataRepo(t *testing.T) { + tests := []struct { + name string + meta map[string]interface{} + wantOwner string + wantRepo string + wantHost string + wantFound bool + wantErr string + }{ + { + name: "parses repo url metadata", + meta: map[string]interface{}{ + "github-repo": "https://github.com/monalisa/octocat-skills", + }, + wantOwner: "monalisa", + wantRepo: "octocat-skills", + wantHost: SupportedHost, + wantFound: true, + }, + { + name: "invalid repo url", + meta: map[string]interface{}{ + "github-repo": "not a url", + }, + wantFound: true, + wantErr: "invalid repository URL", + }, + { + name: "missing repo metadata", + meta: map[string]interface{}{}, + wantFound: false, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + repo, found, err := ParseMetadataRepo(tt.meta) + assert.Equal(t, tt.wantFound, found) + if !tt.wantFound { + require.NoError(t, err) + assert.Nil(t, repo) + return + } + if tt.wantErr != "" { + require.Error(t, err) + assert.Contains(t, err.Error(), tt.wantErr) + return + } + + require.NoError(t, err) + require.NotNil(t, repo) + assert.Equal(t, tt.wantOwner, repo.RepoOwner()) + assert.Equal(t, tt.wantRepo, repo.RepoName()) + assert.Equal(t, tt.wantHost, repo.RepoHost()) + }) + } +} + +func TestValidateSupportedHost(t *testing.T) { + require.NoError(t, ValidateSupportedHost("github.com")) + require.ErrorContains(t, ValidateSupportedHost("acme.ghes.com"), "supports only github.com") +} diff --git a/pkg/cmd/skills/install/install.go b/pkg/cmd/skills/install/install.go index b0ca4bf4a..0cc5c6131 100644 --- a/pkg/cmd/skills/install/install.go +++ b/pkg/cmd/skills/install/install.go @@ -20,6 +20,7 @@ import ( "github.com/cli/cli/v2/internal/skills/frontmatter" "github.com/cli/cli/v2/internal/skills/installer" "github.com/cli/cli/v2/internal/skills/registry" + "github.com/cli/cli/v2/internal/skills/source" "github.com/cli/cli/v2/internal/text" "github.com/cli/cli/v2/pkg/cmdutil" "github.com/cli/cli/v2/pkg/iostreams" @@ -126,10 +127,11 @@ func NewCmdInstall(f *cmdutil.Factory, runF func(*installOptions) error) *cobra. name or use the %[1]s--pin%[1]s flag. The version is resolved as a git tag or commit SHA. Installed skills have GitHub tracking metadata injected into their - frontmatter (%[1]sgithub-owner%[1]s, %[1]sgithub-repo%[1]s, %[1]sgithub-ref%[1]s, - %[1]sgithub-sha%[1]s, %[1]sgithub-tree-sha%[1]s, %[1]sgithub-path%[1]s). This + frontmatter (%[1]sgithub-repo%[1]s, %[1]sgithub-ref%[1]s, + %[1]sgithub-tree-sha%[1]s, %[1]sgithub-path%[1]s). This metadata identifies the source repository and enables %[1]sgh skills update%[1]s to detect changes — the tree SHA serves as an ETag for staleness checks. + The %[1]sgithub-repo%[1]s value is stored as a full repository URL. When run interactively, the command prompts for any missing arguments. When run non-interactively, %[1]srepository%[1]s is required, and either a @@ -226,12 +228,12 @@ func installRun(opts *installOptions) error { return runLocalInstall(opts) } - repo, source, err := resolveRepoArg(opts.SkillSource, canPrompt, opts.Prompter) + repo, repoSource, err := resolveRepoArg(opts.SkillSource, canPrompt, opts.Prompter) if err != nil { return err } opts.repo = repo - opts.SkillSource = source + opts.SkillSource = repoSource parseSkillFromOpts(opts) @@ -242,6 +244,9 @@ func installRun(opts *installOptions) error { apiClient := api.NewClientFromHTTP(httpClient) hostname := opts.repo.RepoHost() + if err := source.ValidateSupportedHost(hostname); err != nil { + return err + } resolved, err := resolveVersion(opts, apiClient, hostname) if err != nil { @@ -290,7 +295,7 @@ func installRun(opts *installOptions) error { gitRoot := installer.ResolveGitRoot(opts.GitClient) homeDir := installer.ResolveHomeDir() - source = ghrepo.FullName(opts.repo) + repoSource = ghrepo.FullName(opts.repo) plans, err := buildInstallPlans(opts, selectedSkills, selectedHosts, scope, gitRoot, homeDir, canPrompt) if err != nil { @@ -322,11 +327,11 @@ func installRun(opts *installOptions) error { for _, name := range result.Installed { fmt.Fprintf(opts.IO.Out, "%s Installed %s (from %s@%s) in %s\n", - cs.SuccessIcon(), name, source, resolved.Ref, friendlyDir(result.Dir)) + cs.SuccessIcon(), name, repoSource, resolved.Ref, friendlyDir(result.Dir)) } printFileTree(opts.IO.Out, cs, result.Dir, result.Installed) - printReviewHint(opts.IO.ErrOut, cs, source, result.Installed) + printReviewHint(opts.IO.ErrOut, cs, repoSource, result.Installed) } if err != nil { @@ -914,16 +919,18 @@ func existingSkillPrompt(targetDir string, incoming discovery.Skill) string { return fmt.Sprintf("Skill %q already exists. Overwrite?", incoming.DisplayName()) } - owner, _ := result.Metadata.Meta["github-owner"].(string) - repo, _ := result.Metadata.Meta["github-repo"].(string) + repoInfo, _, err := source.ParseMetadataRepo(result.Metadata.Meta) ref, _ := result.Metadata.Meta["github-ref"].(string) + if err != nil { + return fmt.Sprintf("Skill %q already exists. Overwrite?", incoming.DisplayName()) + } - if owner != "" && repo != "" { - source := owner + "/" + repo + if repoInfo != nil { + sourceName := ghrepo.FullName(repoInfo) if ref != "" { - source += "@" + ref + sourceName += "@" + ref } - return fmt.Sprintf("Skill %q already installed from %s. Overwrite?", incoming.DisplayName(), source) + return fmt.Sprintf("Skill %q already installed from %s. Overwrite?", incoming.DisplayName(), sourceName) } return fmt.Sprintf("Skill %q already exists. Overwrite?", incoming.DisplayName()) diff --git a/pkg/cmd/skills/install/install_test.go b/pkg/cmd/skills/install/install_test.go index c753ae51c..060b146a4 100644 --- a/pkg/cmd/skills/install/install_test.go +++ b/pkg/cmd/skills/install/install_test.go @@ -1049,8 +1049,7 @@ func TestInstallRun(t *testing.T) { name: git-commit description: Writes commits metadata: - github-owner: someowner - github-repo: somerepo + github-repo: https://github.com/someowner/somerepo github-ref: v0.5.0 --- # Git Commit @@ -1077,6 +1076,22 @@ func TestInstallRun(t *testing.T) { }, wantStdout: "Installed git-commit", }, + { + name: "unsupported host returns error", + stubs: func(reg *httpmock.Registry) {}, + opts: func(ios *iostreams.IOStreams, reg *httpmock.Registry) *installOptions { + t.Helper() + return &installOptions{ + IO: ios, + HttpClient: func() (*http.Client, error) { return &http.Client{Transport: reg}, nil }, + Prompter: &prompter.PrompterMock{}, + GitClient: &git.Client{RepoDir: t.TempDir()}, + SkillSource: "acme.ghes.com/monalisa/octocat-skills", + SkillName: "git-commit", + } + }, + wantErr: "supports only github.com", + }, { name: "select all skills in interactive prompt", isTTY: true, diff --git a/pkg/cmd/skills/preview/preview.go b/pkg/cmd/skills/preview/preview.go index ce7c49ef2..55ba125e3 100644 --- a/pkg/cmd/skills/preview/preview.go +++ b/pkg/cmd/skills/preview/preview.go @@ -14,6 +14,7 @@ import ( "github.com/cli/cli/v2/internal/prompter" "github.com/cli/cli/v2/internal/skills/discovery" "github.com/cli/cli/v2/internal/skills/frontmatter" + "github.com/cli/cli/v2/internal/skills/source" "github.com/cli/cli/v2/pkg/cmdutil" "github.com/cli/cli/v2/pkg/iostreams" "github.com/cli/cli/v2/pkg/markdown" @@ -99,6 +100,9 @@ func previewRun(opts *previewOptions) error { owner := repo.RepoOwner() repoName := repo.RepoName() hostname := repo.RepoHost() + if err := source.ValidateSupportedHost(hostname); err != nil { + return err + } httpClient, err := opts.HttpClient() if err != nil { diff --git a/pkg/cmd/skills/preview/preview_test.go b/pkg/cmd/skills/preview/preview_test.go index 0cbea6ae7..cd623fa06 100644 --- a/pkg/cmd/skills/preview/preview_test.go +++ b/pkg/cmd/skills/preview/preview_test.go @@ -283,6 +283,16 @@ func TestPreviewRun(t *testing.T) { } } +func TestPreviewRun_UnsupportedHost(t *testing.T) { + ios, _, _, _ := iostreams.Test() + err := previewRun(&previewOptions{ + IO: ios, + HttpClient: func() (*http.Client, error) { return &http.Client{}, nil }, + repo: ghrepo.NewWithHost("github", "awesome-copilot", "acme.ghes.com"), + }) + require.ErrorContains(t, err, "supports only github.com") +} + func TestPreviewRun_Interactive(t *testing.T) { skillContent := "# Selected Skill\n\nContent here." encodedContent := base64.StdEncoding.EncodeToString([]byte(skillContent)) diff --git a/pkg/cmd/skills/publish/publish.go b/pkg/cmd/skills/publish/publish.go index 200e3e2dd..96683bece 100644 --- a/pkg/cmd/skills/publish/publish.go +++ b/pkg/cmd/skills/publish/publish.go @@ -16,12 +16,12 @@ import ( "github.com/cli/cli/v2/api" "github.com/cli/cli/v2/git" "github.com/cli/cli/v2/internal/gh" - "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" "github.com/cli/cli/v2/internal/skills/frontmatter" "github.com/cli/cli/v2/internal/skills/registry" + "github.com/cli/cli/v2/internal/skills/source" "github.com/cli/cli/v2/pkg/cmdutil" "github.com/cli/cli/v2/pkg/iostreams" "github.com/spf13/cobra" @@ -342,10 +342,27 @@ func publishRun(opts *publishOptions) error { diagnostics = append(diagnostics, installedDirDiags...) // Remote repository checks (best-effort) - owner, repo := detectGitHubRemote(opts.GitClient) + repoInfo, remoteErr := detectGitHubRemote(opts.GitClient) + if remoteErr != nil { + return remoteErr + } + owner, repo := "", "" + if repoInfo != nil { + owner = repoInfo.RepoOwner() + repo = repoInfo.RepoName() + } hasTopic := false var existingTags []tagEntry if owner != "" && repo != "" { + if host == "" && repoInfo != nil { + host = repoInfo.RepoHost() + } + if host != "" { + if err := source.ValidateSupportedHost(host); err != nil { + return err + } + } + // Create API client for remote checks if not already injected if client == nil { httpClient, httpErr := opts.HttpClient() @@ -354,6 +371,9 @@ func publishRun(opts *publishOptions) error { cfg, cfgErr := opts.Config() if cfgErr == nil { host, _ = cfg.Authentication().DefaultHost() + if err := source.ValidateSupportedHost(host); err != nil { + return err + } client = apiClient } } @@ -844,53 +864,59 @@ func suggestNextTag(latest string) string { } // detectGitHubRemote attempts to detect the GitHub owner/repo from git remotes. -func detectGitHubRemote(gitClient *git.Client) (owner, repo string) { +func detectGitHubRemote(gitClient *git.Client) (ghrepo.Interface, error) { if gitClient == nil { - return "", "" + return nil, nil } // Try origin first if url, err := gitClient.RemoteURL(context.Background(), "origin"); err == nil { - if o, r := parseGitHubURL(url); o != "" { - return o, r + repo, parseErr := parseGitHubURL(url) + if parseErr != nil { + return nil, parseErr + } + if repo != nil { + return repo, nil } } // Fall back to any remote that points to GitHub remotes, err := gitClient.Remotes(context.Background()) if err != nil { - return "", "" + return nil, nil } for _, r := range remotes { if r.Name == "origin" { continue } if url, err := gitClient.RemoteURL(context.Background(), r.Name); err == nil { - if o, rp := parseGitHubURL(url); o != "" { - return o, rp + repo, parseErr := parseGitHubURL(url) + if parseErr != nil { + return nil, parseErr + } + if repo != nil { + return repo, nil } } } - return "", "" + return nil, nil } // parseGitHubURL extracts owner/repo from a GitHub remote URL. // Only GitHub.com URLs are recognized. -func parseGitHubURL(rawURL string) (owner, repo string) { +func parseGitHubURL(rawURL string) (ghrepo.Interface, error) { u, err := git.ParseURL(rawURL) if err != nil { - return "", "" + return nil, nil } r, err := ghrepo.FromURL(u) if err != nil { - return "", "" + return nil, nil } - // Only match github.com — the default GitHub host. - host := strings.ToLower(r.RepoHost()) - if host != ghinstance.Default() { - return "", "" + if err := source.ValidateSupportedHost(r.RepoHost()); err != nil { + return nil, nil } - return r.RepoOwner(), r.RepoName() + return r, nil } // detectMissingRepoDiagnostic explains why remote checks were skipped. diff --git a/pkg/cmd/skills/publish/publish_test.go b/pkg/cmd/skills/publish/publish_test.go index 862dbc9f7..d54d04ad3 100644 --- a/pkg/cmd/skills/publish/publish_test.go +++ b/pkg/cmd/skills/publish/publish_test.go @@ -140,6 +140,27 @@ func TestNewCmdPublish(t *testing.T) { } } +func TestPublishRun_UnsupportedHost(t *testing.T) { + dir := t.TempDir() + writeSkill(t, dir, "test-skill", heredoc.Doc(` + --- + name: test-skill + description: A test skill + --- + Body. + `)) + + ios, _, _, _ := iostreams.Test() + err := publishRun(&publishOptions{ + IO: ios, + Dir: dir, + GitClient: newTestGitClient(t, map[string]string{"origin": "https://github.com/monalisa/skills-repo.git"}), + client: api.NewClientFromHTTP(&http.Client{}), + host: "acme.ghes.com", + }) + require.ErrorContains(t, err, "supports only github.com") +} + func TestPublishRun(t *testing.T) { tests := []struct { name string diff --git a/pkg/cmd/skills/search/search.go b/pkg/cmd/skills/search/search.go index 6a0cc95d0..03874c30c 100644 --- a/pkg/cmd/skills/search/search.go +++ b/pkg/cmd/skills/search/search.go @@ -19,6 +19,7 @@ import ( "github.com/cli/cli/v2/internal/skills/discovery" "github.com/cli/cli/v2/internal/skills/frontmatter" "github.com/cli/cli/v2/internal/skills/registry" + "github.com/cli/cli/v2/internal/skills/source" "github.com/cli/cli/v2/internal/tableprinter" "github.com/cli/cli/v2/internal/text" "github.com/cli/cli/v2/pkg/cmdutil" @@ -200,6 +201,9 @@ func searchRun(opts *searchOptions) error { return err } host, _ := cfg.Authentication().DefaultHost() + if err := source.ValidateSupportedHost(host); err != nil { + return err + } opts.IO.StartProgressIndicatorWithLabel("Searching for skills") diff --git a/pkg/cmd/skills/search/search_test.go b/pkg/cmd/skills/search/search_test.go index e3b8b26d6..6e35465cd 100644 --- a/pkg/cmd/skills/search/search_test.go +++ b/pkg/cmd/skills/search/search_test.go @@ -15,6 +15,25 @@ import ( "github.com/stretchr/testify/require" ) +func TestSearchRun_UnsupportedHost(t *testing.T) { + ios, _, _, _ := iostreams.Test() + cfg := config.NewBlankConfig() + authCfg := cfg.Authentication() + authCfg.SetDefaultHost("acme.ghes.com", "user") + cfg.AuthenticationFunc = func() gh.AuthConfig { + return authCfg + } + err := searchRun(&searchOptions{ + IO: ios, + Query: "terraform", + Page: 1, + Limit: defaultLimit, + HttpClient: func() (*http.Client, error) { return &http.Client{}, nil }, + Config: func() (gh.Config, error) { return cfg, nil }, + }) + require.ErrorContains(t, err, "supports only github.com") +} + func TestNewCmdSearch(t *testing.T) { tests := []struct { name string diff --git a/pkg/cmd/skills/update/update.go b/pkg/cmd/skills/update/update.go index afb8377e3..0a9d1b1fa 100644 --- a/pkg/cmd/skills/update/update.go +++ b/pkg/cmd/skills/update/update.go @@ -17,6 +17,7 @@ import ( "github.com/cli/cli/v2/internal/skills/frontmatter" "github.com/cli/cli/v2/internal/skills/installer" "github.com/cli/cli/v2/internal/skills/registry" + "github.com/cli/cli/v2/internal/skills/source" "github.com/cli/cli/v2/pkg/cmdutil" "github.com/cli/cli/v2/pkg/iostreams" "github.com/spf13/cobra" @@ -43,15 +44,17 @@ type updateOptions struct { // installedSkill represents a locally installed skill parsed from its SKILL.md frontmatter. type installedSkill struct { - name string - owner string - repo string - treeSHA string // tree SHA at install time - pinned string // explicit pin value (empty = unpinned) - sourcePath string // original path in source repo (e.g. "skills/author/name") - dir string // local directory path - host *registry.AgentHost - scope registry.Scope + name string + repoHost string + owner string + repo string + treeSHA string // tree SHA at install time + pinned string // explicit pin value (empty = unpinned) + sourcePath string // original path in source repo (e.g. "skills/author/name") + dir string // local directory path + host *registry.AgentHost + scope registry.Scope + metadataErr error } // pendingUpdate describes a single skill that has an available update. @@ -149,12 +152,6 @@ func updateRun(opts *updateOptions) error { } apiClient := api.NewClientFromHTTP(httpClient) - cfg, err := opts.Config() - if err != nil { - return err - } - hostname, _ := cfg.Authentication().DefaultHost() - gitRoot := installer.ResolveGitRoot(opts.GitClient) homeDir := installer.ResolveHomeDir() @@ -193,6 +190,12 @@ func updateRun(opts *updateOptions) error { installed = filtered } + for _, s := range installed { + if s.metadataErr != nil { + return fmt.Errorf("skill %s has invalid repository metadata: %w", s.name, s.metadataErr) + } + } + // Prompt for metadata on skills missing it (before starting progress indicator) var noMeta []string // Track skills where the user provided a source repo interactively. @@ -226,6 +229,7 @@ func updateRun(opts *updateOptions) error { } s.owner = owner s.repo = repo + s.repoHost = source.SupportedHost prompted[s.dir] = promptedEntry{name: s.name, source: owner + "/" + repo} } @@ -234,7 +238,7 @@ func updateRun(opts *updateOptions) error { var updates []pendingUpdate var pinned []installedSkill - type repoKey struct{ owner, repo string } + type repoKey struct{ host, owner, repo string } repoSkills := make(map[repoKey][]discovery.Skill) repoRefs := make(map[repoKey]*discovery.ResolvedRef) repoErrors := make(map[repoKey]bool) @@ -248,7 +252,7 @@ func updateRun(opts *updateOptions) error { continue } - key := repoKey{s.owner, s.repo} + key := repoKey{s.repoHost, s.owner, s.repo} if repoErrors[key] { continue @@ -256,7 +260,7 @@ func updateRun(opts *updateOptions) error { // Resolve ref and discover skills once per repo if _, ok := repoRefs[key]; !ok { - resolved, resolveErr := discovery.ResolveRef(apiClient, hostname, s.owner, s.repo, "") + resolved, resolveErr := discovery.ResolveRef(apiClient, s.repoHost, s.owner, s.repo, "") if resolveErr != nil { repoErrors[key] = true opts.IO.StopProgressIndicator() @@ -266,7 +270,7 @@ func updateRun(opts *updateOptions) error { } repoRefs[key] = resolved - skills, discoverErr := discovery.DiscoverSkills(apiClient, hostname, s.owner, s.repo, resolved.SHA) + skills, discoverErr := discovery.DiscoverSkills(apiClient, s.repoHost, s.owner, s.repo, resolved.SHA) if discoverErr != nil { repoErrors[key] = true opts.IO.StopProgressIndicator() @@ -302,7 +306,7 @@ func updateRun(opts *updateOptions) error { // Warn about prompted skills that weren't found in the remote repo for _, entry := range prompted { parts := strings.SplitN(entry.source, "/", 2) - key := repoKey{parts[0], parts[1]} + key := repoKey{source.SupportedHost, parts[0], parts[1]} skills, resolved := repoSkills[key] if !resolved { continue @@ -371,7 +375,7 @@ func updateRun(opts *updateOptions) error { var failed bool for _, u := range updates { installOpts := &installer.Options{ - Host: hostname, + Host: u.local.repoHost, Owner: u.local.owner, Repo: u.local.repo, Ref: u.resolved.Ref, @@ -507,8 +511,18 @@ func parseInstalledSkill(data []byte, name, dir string, host *registry.AgentHost } if result.Metadata.Meta != nil { - s.owner, _ = result.Metadata.Meta["github-owner"].(string) - s.repo, _ = result.Metadata.Meta["github-repo"].(string) + repoInfo, ok, repoErr := source.ParseMetadataRepo(result.Metadata.Meta) + if repoErr != nil { + s.metadataErr = repoErr + } else if ok { + if err := source.ValidateSupportedHost(repoInfo.RepoHost()); err != nil { + s.metadataErr = err + } else { + s.repoHost = repoInfo.RepoHost() + s.owner = repoInfo.RepoOwner() + s.repo = repoInfo.RepoName() + } + } s.treeSHA, _ = result.Metadata.Meta["github-tree-sha"].(string) s.pinned, _ = result.Metadata.Meta["github-pinned"].(string) s.sourcePath, _ = result.Metadata.Meta["github-path"].(string) diff --git a/pkg/cmd/skills/update/update_test.go b/pkg/cmd/skills/update/update_test.go index 0c7953f66..b9aabe86a 100644 --- a/pkg/cmd/skills/update/update_test.go +++ b/pkg/cmd/skills/update/update_test.go @@ -91,8 +91,7 @@ func TestScanInstalledSkills(t *testing.T) { name: git-commit description: Git commit helper metadata: - github-owner: monalisa - github-repo: awesome-copilot + github-repo: https://github.com/monalisa/awesome-copilot github-tree-sha: abc123 github-path: skills/git-commit --- @@ -117,8 +116,7 @@ func TestScanInstalledSkills(t *testing.T) { --- name: pinned-skill metadata: - github-owner: octocat - github-repo: hubot-skills + github-repo: https://github.com/octocat/hubot-skills github-tree-sha: def456 github-pinned: v1.0.0 --- @@ -138,6 +136,7 @@ func TestScanInstalledSkills(t *testing.T) { gc := byName["git-commit"] assert.Equal(t, "monalisa", gc.owner) assert.Equal(t, "awesome-copilot", gc.repo) + assert.Equal(t, "github.com", gc.repoHost) assert.Equal(t, "abc123", gc.treeSHA) assert.Equal(t, "skills/git-commit", gc.sourcePath) assert.Empty(t, gc.pinned) @@ -147,9 +146,34 @@ func TestScanInstalledSkills(t *testing.T) { assert.Empty(t, us.repo) ps := byName["pinned-skill"] + assert.Equal(t, "github.com", ps.repoHost) assert.Equal(t, "v1.0.0", ps.pinned) }, }, + { + name: "unsupported host metadata returns error", + setup: func(t *testing.T, dir string) { + t.Helper() + skillDir := filepath.Join(dir, "enterprise-skill") + require.NoError(t, os.MkdirAll(skillDir, 0o755)) + require.NoError(t, os.WriteFile(filepath.Join(skillDir, "SKILL.md"), []byte(heredoc.Doc(` + --- + name: enterprise-skill + metadata: + github-repo: https://acme.ghes.com/monalisa/octocat-skills + github-tree-sha: abc123 + --- + body + `)), 0o644)) + }, + verify: func(t *testing.T, skills []installedSkill, err error) { + t.Helper() + require.NoError(t, err) + require.Len(t, skills, 1) + require.Error(t, skills[0].metadataErr) + assert.Contains(t, skills[0].metadataErr.Error(), "supports only github.com") + }, + }, { name: "non-existent directory returns nil", // no setup — dir does not exist @@ -254,8 +278,7 @@ func TestScanAllAgentsDeduplicatesSharedProjectDirs(t *testing.T) { --- name: git-commit metadata: - github-owner: monalisa - github-repo: octocat-skills + github-repo: https://github.com/monalisa/octocat-skills github-tree-sha: abc123 --- Body @@ -267,8 +290,7 @@ func TestScanAllAgentsDeduplicatesSharedProjectDirs(t *testing.T) { --- name: code-review metadata: - github-owner: monalisa - github-repo: octocat-skills + github-repo: https://github.com/monalisa/octocat-skills github-tree-sha: def456 --- Body @@ -309,8 +331,7 @@ func TestUpdateRun(t *testing.T) { --- name: code-review metadata: - github-owner: monalisa - github-repo: octocat-skills + github-repo: https://github.com/monalisa/octocat-skills github-tree-sha: currentsha github-path: skills/code-review --- @@ -368,8 +389,7 @@ func TestUpdateRun(t *testing.T) { --- name: octocat-skill metadata: - github-owner: octocat - github-repo: hubot-skills + github-repo: https://github.com/octocat/hubot-skills github-tree-sha: abc --- `)), 0o644)) @@ -400,8 +420,7 @@ func TestUpdateRun(t *testing.T) { --- name: pinned-skill metadata: - github-owner: octocat - github-repo: hubot-skills + github-repo: https://github.com/octocat/hubot-skills github-tree-sha: abc123 github-pinned: v1.0.0 --- @@ -463,8 +482,7 @@ func TestUpdateRun(t *testing.T) { --- name: monalisa-skill metadata: - github-owner: monalisa - github-repo: octocat-skills + github-repo: https://github.com/monalisa/octocat-skills github-tree-sha: abc123def456 github-path: skills/monalisa-skill --- @@ -508,8 +526,7 @@ func TestUpdateRun(t *testing.T) { --- name: hubot-skill metadata: - github-owner: hubot - github-repo: octocat-skills + github-repo: https://github.com/hubot/octocat-skills github-tree-sha: oldsha123 github-path: skills/hubot-skill --- @@ -557,8 +574,7 @@ func TestUpdateRun(t *testing.T) { --- name: hubot-skill metadata: - github-owner: hubot - github-repo: octocat-skills + github-repo: https://github.com/hubot/octocat-skills github-tree-sha: oldsha123 github-path: skills/hubot-skill --- @@ -606,8 +622,7 @@ func TestUpdateRun(t *testing.T) { --- name: code-review metadata: - github-owner: monalisa - github-repo: octocat-skills + github-repo: https://github.com/monalisa/octocat-skills github-tree-sha: oldsha000 github-path: skills/code-review --- @@ -650,7 +665,7 @@ func TestUpdateRun(t *testing.T) { t.Helper() content, err := os.ReadFile(filepath.Join(dir, "code-review", "SKILL.md")) require.NoError(t, err) - assert.Contains(t, string(content), "github-owner: monalisa") + assert.Contains(t, string(content), "github-repo: https://github.com/monalisa/octocat-skills") assert.NotContains(t, string(content), "Old content") }, wantStdout: "Updated code-review", @@ -668,8 +683,7 @@ func TestUpdateRun(t *testing.T) { --- name: code-review metadata: - github-owner: monalisa - github-repo: octocat-skills + github-repo: https://github.com/monalisa/octocat-skills github-tree-sha: oldsha000 github-path: skills/monalisa/code-review --- @@ -712,7 +726,7 @@ func TestUpdateRun(t *testing.T) { t.Helper() content, err := os.ReadFile(filepath.Join(dir, "monalisa", "code-review", "SKILL.md")) require.NoError(t, err) - assert.Contains(t, string(content), "github-owner: monalisa") + assert.Contains(t, string(content), "github-repo: https://github.com/monalisa/octocat-skills") assert.NotContains(t, string(content), "Old namespaced content") }, wantStdout: "Updated monalisa/code-review", @@ -730,8 +744,7 @@ func TestUpdateRun(t *testing.T) { --- name: code-review metadata: - github-owner: monalisa - github-repo: octocat-skills + github-repo: https://github.com/monalisa/octocat-skills github-tree-sha: oldsha000 github-path: skills/code-review --- @@ -790,8 +803,7 @@ func TestUpdateRun(t *testing.T) { --- name: code-review metadata: - github-owner: monalisa - github-repo: octocat-skills + github-repo: https://github.com/monalisa/octocat-skills github-tree-sha: oldsha000 github-path: skills/code-review --- @@ -853,8 +865,7 @@ func TestUpdateRun(t *testing.T) { --- name: code-review metadata: - github-owner: monalisa - github-repo: octocat-skills + github-repo: https://github.com/monalisa/octocat-skills github-tree-sha: oldsha000 github-path: skills/code-review --- @@ -990,7 +1001,7 @@ func TestUpdateRun(t *testing.T) { content, err := os.ReadFile(filepath.Join(dir, "manual-skill", "SKILL.md")) require.NoError(t, err) assert.NotContains(t, string(content), "Old manual content") - assert.Contains(t, string(content), "github-owner: monalisa") + assert.Contains(t, string(content), "github-repo: https://github.com/monalisa/octocat-skills") }, wantStdout: "Updated manual-skill", }, @@ -1007,8 +1018,7 @@ func TestUpdateRun(t *testing.T) { --- name: pinned-skill metadata: - github-owner: octocat - github-repo: hubot-skills + github-repo: https://github.com/octocat/hubot-skills github-tree-sha: oldsha000 github-pinned: v1.0.0 github-path: skills/pinned-skill @@ -1067,8 +1077,7 @@ func TestUpdateRun(t *testing.T) { --- name: pinned-skill metadata: - github-owner: octocat - github-repo: hubot-skills + github-repo: https://github.com/octocat/hubot-skills github-tree-sha: abc123 github-pinned: v1.0.0 --- @@ -1102,8 +1111,7 @@ func TestUpdateRun(t *testing.T) { --- name: pinned-skill metadata: - github-owner: octocat - github-repo: hubot-skills + github-repo: https://github.com/octocat/hubot-skills github-tree-sha: oldsha000 github-pinned: v1.0.0 github-path: skills/pinned-skill