diff --git a/acceptance/testdata/skills/skills-install-alias.txtar b/acceptance/testdata/skills/skills-install-alias.txtar deleted file mode 100644 index 089474b3a..000000000 --- a/acceptance/testdata/skills/skills-install-alias.txtar +++ /dev/null @@ -1,3 +0,0 @@ -# Install with "add" alias -exec gh skills add github/awesome-copilot git-commit --scope user --force --agent github-copilot -stdout 'Installed git-commit' diff --git a/acceptance/testdata/skills/skills-install-conflict.txtar b/acceptance/testdata/skills/skills-install-conflict.txtar deleted file mode 100644 index 9e79e5a5e..000000000 --- a/acceptance/testdata/skills/skills-install-conflict.txtar +++ /dev/null @@ -1,8 +0,0 @@ -# Install --all should handle skills with same name across conventions -# (skills/ and plugins/ directories) without collision errors -exec gh skills install github/awesome-copilot --all --force --dir $WORK/scope-test --agent github-copilot -stdout 'Installed' -! stderr 'conflicting names' - -# Verify skills were installed successfully -exists $WORK/scope-test/git-commit/SKILL.md diff --git a/acceptance/testdata/skills/skills-install-invalid-agent.txtar b/acceptance/testdata/skills/skills-install-invalid-agent.txtar new file mode 100644 index 000000000..23883524f --- /dev/null +++ b/acceptance/testdata/skills/skills-install-invalid-agent.txtar @@ -0,0 +1,4 @@ +# Invalid agent ID should error with valid options +! exec gh skills install github/awesome-copilot git-commit --agent bogus-agent --force +stderr 'unknown agent' +stderr 'github-copilot' diff --git a/acceptance/testdata/skills/skills-install-invalid-repo.txtar b/acceptance/testdata/skills/skills-install-invalid-repo.txtar new file mode 100644 index 000000000..26ecbc718 --- /dev/null +++ b/acceptance/testdata/skills/skills-install-invalid-repo.txtar @@ -0,0 +1,3 @@ +# Nonexistent repo should error +! exec gh skills install nonexistent-owner-xyz/nonexistent-repo-abc --force --dir $WORK/tmp +stderr 'Not Found' diff --git a/acceptance/testdata/skills/skills-install-nested-files.txtar b/acceptance/testdata/skills/skills-install-nested-files.txtar new file mode 100644 index 000000000..c5cf19e56 --- /dev/null +++ b/acceptance/testdata/skills/skills-install-nested-files.txtar @@ -0,0 +1,3 @@ +# Install a skill that has nested subdirectories and verify file tree +exec gh skills install github/awesome-copilot git-commit --force --dir $WORK/nested-test +exists $WORK/nested-test/git-commit/SKILL.md diff --git a/acceptance/testdata/skills/skills-install-nonexistent-skill.txtar b/acceptance/testdata/skills/skills-install-nonexistent-skill.txtar new file mode 100644 index 000000000..23f72cee8 --- /dev/null +++ b/acceptance/testdata/skills/skills-install-nonexistent-skill.txtar @@ -0,0 +1,3 @@ +# Installing a skill that doesn't exist in a valid repo should error +! exec gh skills install github/awesome-copilot nonexistent-skill-xyz --force --dir $WORK/tmp +stderr 'not found' diff --git a/acceptance/testdata/skills/skills-install-scope.txtar b/acceptance/testdata/skills/skills-install-scope.txtar index 9b8048ab5..da2df19ea 100644 --- a/acceptance/testdata/skills/skills-install-scope.txtar +++ b/acceptance/testdata/skills/skills-install-scope.txtar @@ -1,9 +1,9 @@ -# Install with --scope project (default) inside a git repo -exec git init $WORK/myrepo -exec gh skills install github/awesome-copilot git-commit --scope project --force --agent github-copilot --dir $WORK/myrepo/.github/skills -stdout 'Installed git-commit' +# Install with --scope project writes to the git repo's .github/skills/ +exec git init --initial-branch=main $WORK/myrepo +cd $WORK/myrepo +exec gh skills install github/awesome-copilot git-commit --scope project --force --agent github-copilot exists $WORK/myrepo/.github/skills/git-commit/SKILL.md -# Install with --scope user +# Install with --scope user writes to home directory exec gh skills install github/awesome-copilot git-commit --scope user --force --agent github-copilot -stdout 'Installed git-commit' +exists $HOME/.copilot/skills/git-commit/SKILL.md diff --git a/acceptance/testdata/skills/skills-install.txtar b/acceptance/testdata/skills/skills-install.txtar index c04ced9d2..183f930fd 100644 --- a/acceptance/testdata/skills/skills-install.txtar +++ b/acceptance/testdata/skills/skills-install.txtar @@ -2,9 +2,19 @@ exec gh skills install github/awesome-copilot git-commit --scope user --force --agent github-copilot stdout 'Installed git-commit' +# Verify SKILL.md has frontmatter metadata injected +exists $HOME/.copilot/skills/git-commit/SKILL.md +grep 'github-owner' $HOME/.copilot/skills/git-commit/SKILL.md +grep 'github-repo' $HOME/.copilot/skills/git-commit/SKILL.md + +# Verify lockfile was written +exists $HOME/.agents/.skill-lock.json +grep 'git-commit' $HOME/.agents/.skill-lock.json + # Install with --dir to a custom directory exec gh skills install github/awesome-copilot git-commit --force --dir $WORK/custom-skills stdout 'Installed git-commit' # Verify the skill was written to the custom directory exists $WORK/custom-skills/git-commit/SKILL.md +grep 'github-owner' $WORK/custom-skills/git-commit/SKILL.md diff --git a/acceptance/testdata/skills/skills-preview-noninteractive.txtar b/acceptance/testdata/skills/skills-preview-noninteractive.txtar new file mode 100644 index 000000000..939df0ab6 --- /dev/null +++ b/acceptance/testdata/skills/skills-preview-noninteractive.txtar @@ -0,0 +1,3 @@ +# Preview with repo only and non-interactive should error +! exec gh skills preview github/awesome-copilot +stderr 'must specify a skill name' diff --git a/acceptance/testdata/skills/skills-preview.txtar b/acceptance/testdata/skills/skills-preview.txtar new file mode 100644 index 000000000..3834c340c --- /dev/null +++ b/acceptance/testdata/skills/skills-preview.txtar @@ -0,0 +1,9 @@ +# Preview renders skill content and file tree +exec gh skills preview github/awesome-copilot git-commit +stdout 'SKILL.md' +# Verify actual content is rendered, not just the filename +stdout 'git-commit/' + +# Preview a skill that doesn't exist should error +! exec gh skills preview github/awesome-copilot nonexistent-skill-xyz +stderr 'not found' diff --git a/acceptance/testdata/skills/skills-publish-dry-run.txtar b/acceptance/testdata/skills/skills-publish-dry-run.txtar new file mode 100644 index 000000000..39c0f234d --- /dev/null +++ b/acceptance/testdata/skills/skills-publish-dry-run.txtar @@ -0,0 +1,33 @@ +# Publish dry-run from a directory with no skills/ should fail gracefully +! exec gh skills publish --dry-run $WORK +stderr 'no skills/ directory found' + +# Publish dry-run against a valid skill directory should succeed +exec gh skills publish --dry-run $WORK/test-repo +stdout 'hello-world' + +# Validate alias should work identically +exec gh skills validate --dry-run $WORK/test-repo +stdout 'hello-world' + +# Publish dry-run with --tag +exec gh skills publish --dry-run --tag v1.0.0 $WORK/test-repo +stdout 'hello-world' + +# Publish dry-run with --fix +exec gh skills publish --dry-run --fix $WORK/test-repo +stdout 'hello-world' + +-- test-repo/skills/hello-world/SKILL.md -- +--- +name: hello-world +description: A test skill that greets the user. +--- + +# Hello World + +Greet the user warmly. + +-- test-repo/skills/hello-world/scripts/setup.sh -- +#!/bin/bash +echo "Hello from the hello-world skill!" diff --git a/acceptance/testdata/skills/skills-publish-lifecycle.txtar b/acceptance/testdata/skills/skills-publish-lifecycle.txtar new file mode 100644 index 000000000..0e8a03a1d --- /dev/null +++ b/acceptance/testdata/skills/skills-publish-lifecycle.txtar @@ -0,0 +1,64 @@ +# Full publish lifecycle: create repo, publish, install from it, clean up + +# Use gh as a credential helper +exec gh auth setup-git + +# Create a private repo for testing +exec gh repo create $ORG/$SCRIPT_NAME-$RANDOM_STRING --private --add-readme +defer gh repo delete --yes $ORG/$SCRIPT_NAME-$RANDOM_STRING + +# Clone the repo +exec gh repo clone $ORG/$SCRIPT_NAME-$RANDOM_STRING +cd $SCRIPT_NAME-$RANDOM_STRING + +# Add a test skill +mkdir skills/hello-world/scripts +cp $WORK/skill.md skills/hello-world/SKILL.md +cp $WORK/setup.sh skills/hello-world/scripts/setup.sh +exec git add -A +exec git commit -m 'Add test skill' +exec git push origin main + +# Publish with a tag +exec gh skills publish --tag v0.1.0 + +# Verify the release was created on GitHub +exec gh release view v0.1.0 +stdout 'v0.1.0' + +# Install from our test repo +exec gh skills install $ORG/$SCRIPT_NAME-$RANDOM_STRING hello-world --scope user --force +stdout 'Installed hello-world' + +# Verify installed files exist with correct metadata +exists $HOME/.copilot/skills/hello-world/SKILL.md +exists $HOME/.copilot/skills/hello-world/scripts/setup.sh +grep 'github-owner' $HOME/.copilot/skills/hello-world/SKILL.md + +# Install with --pin +exec gh skills install $ORG/$SCRIPT_NAME-$RANDOM_STRING hello-world --scope user --force --pin v0.1.0 +stdout 'Installed hello-world' + +# Preview from our test repo +exec gh skills preview $ORG/$SCRIPT_NAME-$RANDOM_STRING hello-world +stdout 'Hello World' + +# Update dry-run should find installed skill +exec gh skills update --dry-run --all +stderr 'up to date' + +-- skill.md -- +--- +name: hello-world +description: A test skill that greets the user. +--- + +# Hello World + +Greet the user warmly and offer to run the setup script. + +-- setup.sh -- +#!/bin/bash +echo "Hello from the hello-world skill!" +echo "Setting up environment..." +echo "Done." diff --git a/acceptance/testdata/skills/skills-search-noresults.txtar b/acceptance/testdata/skills/skills-search-noresults.txtar new file mode 100644 index 000000000..31f8293f0 --- /dev/null +++ b/acceptance/testdata/skills/skills-search-noresults.txtar @@ -0,0 +1,4 @@ +# Search for something unlikely to exist returns empty stdout +# (NoResultsError is silent in non-TTY — exits 0 with no output) +exec gh skills search zzzznonexistenttotallyfakeskillxyz123 +! stdout . diff --git a/acceptance/testdata/skills/skills-search-page.txtar b/acceptance/testdata/skills/skills-search-page.txtar new file mode 100644 index 000000000..71bc6f1de --- /dev/null +++ b/acceptance/testdata/skills/skills-search-page.txtar @@ -0,0 +1,3 @@ +# Pagination returns results on page 2 +exec gh skills search copilot --page 2 +stdout 'copilot' diff --git a/acceptance/testdata/skills/skills-search.txtar b/acceptance/testdata/skills/skills-search.txtar new file mode 100644 index 000000000..eb4759a41 --- /dev/null +++ b/acceptance/testdata/skills/skills-search.txtar @@ -0,0 +1,12 @@ +# Search for skills matching a query +exec gh skills search copilot +stdout 'copilot' + +# Search with JSON output +exec gh skills search copilot --json skillName,repo --limit 1 +stdout '"skillName"' +stdout '"repo"' + +# Search with a short query should error +! exec gh skills search a +stderr 'at least' diff --git a/acceptance/testdata/skills/skills-update-noinstalled.txtar b/acceptance/testdata/skills/skills-update-noinstalled.txtar new file mode 100644 index 000000000..7f24291ba --- /dev/null +++ b/acceptance/testdata/skills/skills-update-noinstalled.txtar @@ -0,0 +1,5 @@ +# Update with no installed skills should report appropriately +exec gh skills update --dry-run --all --dir $WORK/empty-dir +stderr 'No installed skills found' + +-- empty-dir/.gitkeep -- diff --git a/acceptance/testdata/skills/skills-update.txtar b/acceptance/testdata/skills/skills-update.txtar new file mode 100644 index 000000000..7041c84b4 --- /dev/null +++ b/acceptance/testdata/skills/skills-update.txtar @@ -0,0 +1,24 @@ +# Dry-run update should find the installed skill and report status +exec gh skills update --dry-run --all --dir $WORK/skills-dir +stderr 'update' +stdout 'git-commit' + +# Force update should re-download and rewrite files +exec gh skills update --force --all --dir $WORK/skills-dir +stdout 'Updated' + +# Verify the SKILL.md was rewritten with real content (not our placeholder) +grep 'github-owner' $WORK/skills-dir/git-commit/SKILL.md +! grep 'Test skill content' $WORK/skills-dir/git-commit/SKILL.md + +-- skills-dir/git-commit/SKILL.md -- +--- +name: git-commit +description: Git commit helper +metadata: + github-owner: github + github-repo: awesome-copilot + github-tree-sha: 0000000000000000000000000000000000000000 + github-path: skills/git-commit +--- +Test skill content diff --git a/git/client.go b/git/client.go index 5f547c99c..22c4eff16 100644 --- a/git/client.go +++ b/git/client.go @@ -713,6 +713,37 @@ func (c *Client) IsLocalGitRepo(ctx context.Context) (bool, error) { return true, nil } +// RemoteURL returns the fetch URL configured for the named remote. +func (c *Client) RemoteURL(ctx context.Context, name string) (string, error) { + cmd, err := c.Command(ctx, "remote", "get-url", name) + if err != nil { + return "", err + } + out, err := cmd.Output() + if err != nil { + return "", err + } + return firstLine(out), nil +} + +// IsIgnored reports whether the given path is ignored by .gitignore rules. +func (c *Client) IsIgnored(ctx context.Context, path string) bool { + cmd, err := c.Command(ctx, "check-ignore", "-q", path) + if err != nil { + return false + } + _, err = cmd.Output() + return err == nil +} + +// ShortSHA returns the first 8 characters of a SHA hash for display purposes. +func ShortSHA(sha string) string { + if len(sha) > 8 { + return sha[:8] + } + return sha +} + func (c *Client) UnsetRemoteResolution(ctx context.Context, name string) error { args := []string{"config", "--unset", fmt.Sprintf("remote.%s.gh-resolved", name)} cmd, err := c.Command(ctx, args...) diff --git a/internal/skills/collisions.go b/internal/skills/discovery/collisions.go similarity index 89% rename from internal/skills/collisions.go rename to internal/skills/discovery/collisions.go index 87e4705c9..38bf9b26b 100644 --- a/internal/skills/collisions.go +++ b/internal/skills/discovery/collisions.go @@ -1,11 +1,9 @@ -package skills +package discovery import ( "fmt" "sort" "strings" - - "github.com/cli/cli/v2/internal/skills/discovery" ) // NameCollision represents a group of skills that share the same InstallName @@ -18,8 +16,8 @@ type NameCollision struct { // FindNameCollisions detects skills that share the same InstallName and returns a // sorted slice of collisions. Callers decide how to present the conflict to // the user (different flows need different error messages). -func FindNameCollisions(skills []discovery.Skill) []NameCollision { - byName := make(map[string][]discovery.Skill) +func FindNameCollisions(skills []Skill) []NameCollision { + byName := make(map[string][]Skill) for _, s := range skills { byName[s.InstallName()] = append(byName[s.InstallName()], s) } diff --git a/internal/skills/discovery/collisions_test.go b/internal/skills/discovery/collisions_test.go new file mode 100644 index 000000000..b499c497a --- /dev/null +++ b/internal/skills/discovery/collisions_test.go @@ -0,0 +1,62 @@ +package discovery + +import ( + "testing" + + "github.com/stretchr/testify/assert" +) + +func TestFindNameCollisions(t *testing.T) { + tests := []struct { + name string + skills []Skill + want []NameCollision + }{ + { + name: "no collisions", + skills: []Skill{ + {Name: "code-review", Path: "skills/code-review"}, + {Name: "issue-triage", Path: "skills/issue-triage"}, + }, + want: nil, + }, + { + name: "single collision", + skills: []Skill{ + {Name: "pr-summary", Path: "skills/pr-summary"}, + {Name: "pr-summary", Path: "skills/monalisa/pr-summary"}, + }, + want: []NameCollision{ + {Name: "pr-summary", DisplayNames: []string{"pr-summary", "pr-summary"}}, + }, + }, + { + name: "collisions sorted by name", + skills: []Skill{ + {Name: "octocat-lint", Path: "skills/octocat-lint"}, + {Name: "octocat-lint", Path: "skills/hubot/octocat-lint"}, + {Name: "code-review", Path: "skills/code-review"}, + {Name: "code-review", Path: "skills/monalisa/code-review"}, + }, + want: []NameCollision{ + {Name: "code-review", DisplayNames: []string{"code-review", "code-review"}}, + {Name: "octocat-lint", DisplayNames: []string{"octocat-lint", "octocat-lint"}}, + }, + }, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + got := FindNameCollisions(tt.skills) + assert.Equal(t, tt.want, got) + }) + } +} + +func TestFormatCollisions(t *testing.T) { + collisions := []NameCollision{ + {Name: "pr-summary", DisplayNames: []string{"skills/pr-summary", "plugins/hubot/pr-summary"}}, + {Name: "code-review", DisplayNames: []string{"skills/code-review", "skills/monalisa/code-review"}}, + } + got := FormatCollisions(collisions) + assert.Equal(t, "pr-summary: skills/pr-summary, plugins/hubot/pr-summary\n code-review: skills/code-review, skills/monalisa/code-review", got) +} diff --git a/internal/skills/discovery/discovery.go b/internal/skills/discovery/discovery.go index fc234716a..05a531bc9 100644 --- a/internal/skills/discovery/discovery.go +++ b/internal/skills/discovery/discovery.go @@ -11,6 +11,7 @@ import ( "strings" "sync" + "github.com/cli/cli/v2/api" "github.com/cli/cli/v2/internal/skills/frontmatter" ) @@ -71,18 +72,6 @@ type ResolvedRef struct { SHA string // commit SHA } -// RESTClient is the interface for making GitHub REST API calls. -// It mirrors the subset of api.Client used by discovery. -type RESTClient interface { - // REST performs a REST API call. - // hostname is the GitHub host (e.g. "github.com"). - // method is the HTTP method (e.g. "GET"). - // path is the API path (e.g. "repos/owner/repo/releases/latest"). - // body is the request body (nil for GET). - // data is the response data to unmarshal into. - REST(hostname string, method string, path string, body io.Reader, data interface{}) error -} - type treeEntry struct { Path string `json:"path"` Mode string `json:"mode"` @@ -120,7 +109,7 @@ type repoResponse struct { // ResolveRef determines the git ref to use for a given owner/repo. // Priority: explicit version → latest release tag → default branch. -func ResolveRef(client RESTClient, host, owner, repo, version string) (*ResolvedRef, error) { +func ResolveRef(client *api.Client, host, owner, repo, version string) (*ResolvedRef, error) { if version != "" { return resolveExplicitRef(client, host, owner, repo, version) } @@ -134,7 +123,7 @@ func ResolveRef(client RESTClient, host, owner, repo, version string) (*Resolved // resolveExplicitRef resolves a user-supplied --pin value. It tries, in order: // tag → commit SHA. Branches are deliberately excluded because they are mutable // and pinning to one gives a false sense of reproducibility. -func resolveExplicitRef(client RESTClient, host, owner, repo, ref string) (*ResolvedRef, error) { +func resolveExplicitRef(client *api.Client, host, owner, repo, ref string) (*ResolvedRef, error) { tagPath := fmt.Sprintf("repos/%s/%s/git/ref/tags/%s", owner, repo, ref) var refResp struct { Object struct { @@ -170,7 +159,7 @@ func resolveExplicitRef(client RESTClient, host, owner, repo, ref string) (*Reso return nil, fmt.Errorf("ref %q not found as tag or commit in %s/%s", ref, owner, repo) } -func resolveLatestRelease(client RESTClient, host, owner, repo string) (*ResolvedRef, error) { +func resolveLatestRelease(client *api.Client, host, owner, repo string) (*ResolvedRef, error) { apiPath := fmt.Sprintf("repos/%s/%s/releases/latest", owner, repo) var release releaseResponse if err := client.REST(host, "GET", apiPath, nil, &release); err != nil { @@ -182,7 +171,7 @@ func resolveLatestRelease(client RESTClient, host, owner, repo string) (*Resolve return resolveExplicitRef(client, host, owner, repo, release.TagName) } -func resolveDefaultBranch(client RESTClient, host, owner, repo string) (*ResolvedRef, error) { +func resolveDefaultBranch(client *api.Client, host, owner, repo string) (*ResolvedRef, error) { apiPath := fmt.Sprintf("repos/%s/%s", owner, repo) var repoResp repoResponse if err := client.REST(host, "GET", apiPath, nil, &repoResp); err != nil { @@ -235,7 +224,7 @@ func matchSkillConventions(entry treeEntry) *skillMatch { parentDir := path.Dir(dir) skillName := path.Base(dir) - if !ValidateName(skillName) { + if !validateName(skillName) { return nil } @@ -246,7 +235,7 @@ func matchSkillConventions(entry treeEntry) *skillMatch { grandparentDir := path.Dir(parentDir) if grandparentDir == "skills" { namespace := path.Base(parentDir) - if !ValidateName(namespace) { + if !validateName(namespace) { return nil } return &skillMatch{entry: entry, name: skillName, namespace: namespace, skillDir: dir, convention: "skills-namespaced"} @@ -254,7 +243,7 @@ func matchSkillConventions(entry treeEntry) *skillMatch { if path.Base(parentDir) == "skills" && path.Dir(grandparentDir) == "plugins" { namespace := path.Base(grandparentDir) - if !ValidateName(namespace) { + if !validateName(namespace) { return nil } return &skillMatch{entry: entry, name: skillName, namespace: namespace, skillDir: dir, convention: "plugins"} @@ -268,7 +257,7 @@ func matchSkillConventions(entry treeEntry) *skillMatch { } // DiscoverSkills finds all skills in a repository at the given commit SHA. -func DiscoverSkills(client RESTClient, host, owner, repo, commitSHA string) ([]Skill, error) { +func DiscoverSkills(client *api.Client, host, owner, repo, commitSHA string) ([]Skill, error) { apiPath := fmt.Sprintf("repos/%s/%s/git/trees/%s?recursive=true", owner, repo, commitSHA) var tree treeResponse if err := client.REST(host, "GET", apiPath, nil, &tree); err != nil { @@ -332,8 +321,8 @@ func DiscoverSkills(client RESTClient, host, owner, repo, commitSHA string) ([]S return skills, nil } -// FetchDescription fetches and parses the frontmatter description for a skill. -func FetchDescription(client RESTClient, host, owner, repo string, skill *Skill) string { +// fetchDescription fetches and parses the frontmatter description for a skill. +func fetchDescription(client *api.Client, host, owner, repo string, skill *Skill) string { if skill.BlobSHA == "" { return "" } @@ -348,17 +337,8 @@ func FetchDescription(client RESTClient, host, owner, repo string, skill *Skill) return result.Metadata.Description } -// FetchDescriptions fetches descriptions for a batch of skills. -func FetchDescriptions(client RESTClient, host, owner, repo string, skills []Skill) { - for i := range skills { - if skills[i].Description == "" { - skills[i].Description = FetchDescription(client, host, owner, repo, &skills[i]) - } - } -} - // FetchDescriptionsConcurrent fetches descriptions with bounded concurrency. -func FetchDescriptionsConcurrent(client RESTClient, host, owner, repo string, skills []Skill, onProgress func(done, total int)) { +func FetchDescriptionsConcurrent(client *api.Client, host, owner, repo string, skills []Skill, onProgress func(done, total int)) { total := 0 for _, s := range skills { if s.Description == "" { @@ -385,7 +365,7 @@ func FetchDescriptionsConcurrent(client RESTClient, host, owner, repo string, sk sem <- struct{}{} defer func() { <-sem }() - desc := FetchDescription(client, host, owner, repo, &skills[idx]) + desc := fetchDescription(client, host, owner, repo, &skills[idx]) mu.Lock() skills[idx].Description = desc @@ -401,12 +381,12 @@ func FetchDescriptionsConcurrent(client RESTClient, host, owner, repo string, sk } // DiscoverSkillByPath looks up a single skill by its exact path in the repository. -func DiscoverSkillByPath(client RESTClient, host, owner, repo, commitSHA, skillPath string) (*Skill, error) { +func DiscoverSkillByPath(client *api.Client, host, owner, repo, commitSHA, skillPath string) (*Skill, error) { skillPath = strings.TrimSuffix(skillPath, "/SKILL.md") skillPath = strings.TrimSuffix(skillPath, "/") skillName := path.Base(skillPath) - if !ValidateName(skillName) { + if !validateName(skillName) { return nil, fmt.Errorf("invalid skill name %q", skillName) } @@ -465,14 +445,14 @@ func DiscoverSkillByPath(client RESTClient, host, owner, repo, commitSHA, skillP TreeSHA: treeSHA, } - skill.Description = FetchDescription(client, host, owner, repo, skill) + skill.Description = fetchDescription(client, host, owner, repo, skill) return skill, nil } // DiscoverSkillFiles returns all file paths belonging to a skill directory // by fetching the skill's subtree directly using its tree SHA. -func DiscoverSkillFiles(client RESTClient, host, owner, repo, treeSHA, skillPath string) ([]treeEntry, error) { +func DiscoverSkillFiles(client *api.Client, host, owner, repo, treeSHA, skillPath string) ([]SkillFile, error) { apiPath := fmt.Sprintf("repos/%s/%s/git/trees/%s?recursive=true", owner, repo, treeSHA) var tree treeResponse if err := client.REST(host, "GET", apiPath, nil, &tree); err != nil { @@ -484,10 +464,10 @@ func DiscoverSkillFiles(client RESTClient, host, owner, repo, treeSHA, skillPath return walkTree(client, host, owner, repo, treeSHA, skillPath) } - var files []treeEntry + var files []SkillFile for _, entry := range tree.Tree { if entry.Type == "blob" { - files = append(files, treeEntry{ + files = append(files, SkillFile{ Path: skillPath + "/" + entry.Path, SHA: entry.SHA, Size: entry.Size, @@ -500,7 +480,7 @@ func DiscoverSkillFiles(client RESTClient, host, owner, repo, treeSHA, skillPath // ListSkillFiles returns all files in a skill directory as public SkillFile // structs with paths relative to the skill root. -func ListSkillFiles(client RESTClient, host, owner, repo, treeSHA string) ([]SkillFile, error) { +func ListSkillFiles(client *api.Client, host, owner, repo, treeSHA string) ([]SkillFile, error) { apiPath := fmt.Sprintf("repos/%s/%s/git/trees/%s?recursive=true", owner, repo, treeSHA) var tree treeResponse if err := client.REST(host, "GET", apiPath, nil, &tree); err != nil { @@ -509,17 +489,7 @@ func ListSkillFiles(client RESTClient, host, owner, repo, treeSHA string) ([]Ski if tree.Truncated { // Fall back to non-recursive traversal when the tree is too large. - entries, err := walkTree(client, host, owner, repo, treeSHA, "") - if err != nil { - return nil, err - } - var files []SkillFile - for _, e := range entries { - // walkTree prefixes with "/{path}", trim the leading slash. - p := strings.TrimPrefix(e.Path, "/") - files = append(files, SkillFile{Path: p, SHA: e.SHA, Size: e.Size}) - } - return files, nil + return walkTree(client, host, owner, repo, treeSHA, "") } var files []SkillFile @@ -537,19 +507,22 @@ func ListSkillFiles(client RESTClient, host, owner, repo, treeSHA string) ([]Ski // walkTree enumerates files by fetching each tree level individually, // avoiding the truncation limit of the recursive tree API. -func walkTree(client RESTClient, host, owner, repo, sha, prefix string) ([]treeEntry, error) { +func walkTree(client *api.Client, host, owner, repo, sha, prefix string) ([]SkillFile, error) { apiPath := fmt.Sprintf("repos/%s/%s/git/trees/%s", owner, repo, sha) var tree treeResponse if err := client.REST(host, "GET", apiPath, nil, &tree); err != nil { return nil, fmt.Errorf("could not fetch tree %s: %w", prefix, err) } - var files []treeEntry + var files []SkillFile for _, entry := range tree.Tree { - entryPath := prefix + "/" + entry.Path + entryPath := entry.Path + if prefix != "" { + entryPath = prefix + "/" + entry.Path + } switch entry.Type { case "blob": - files = append(files, treeEntry{Path: entryPath, SHA: entry.SHA, Size: entry.Size}) + files = append(files, SkillFile{Path: entryPath, SHA: entry.SHA, Size: entry.Size}) case "tree": sub, err := walkTree(client, host, owner, repo, entry.SHA, entryPath) if err != nil { @@ -562,7 +535,7 @@ func walkTree(client RESTClient, host, owner, repo, sha, prefix string) ([]treeE } // FetchBlob retrieves the content of a blob by SHA. -func FetchBlob(client RESTClient, host, owner, repo, sha string) (string, error) { +func FetchBlob(client *api.Client, host, owner, repo, sha string) (string, error) { apiPath := fmt.Sprintf("repos/%s/%s/git/blobs/%s", owner, repo, sha) var blob blobResponse if err := client.REST(host, "GET", apiPath, nil, &blob); err != nil { @@ -683,7 +656,7 @@ func localSkillFromDir(dir string) (*Skill, error) { description = result.Metadata.Description } - if !ValidateName(name) { + if !validateName(name) { return nil, fmt.Errorf("invalid skill name %q in %s", name, dir) } @@ -694,8 +667,8 @@ func localSkillFromDir(dir string) (*Skill, error) { }, nil } -// ValidateName checks if a skill name is safe for use (filesystem-safe). -func ValidateName(name string) bool { +// validateName checks if a skill name is safe for use (filesystem-safe). +func validateName(name string) bool { if len(name) == 0 || len(name) > 64 { return false } @@ -715,59 +688,3 @@ func IsSpecCompliant(name string) bool { } return specNamePattern.MatchString(name) } - -// verifyBatchSize controls how many repos are checked per code-search API call. -const verifyBatchSize = 8 - -type codeSearchResponse struct { - Items []codeSearchItem `json:"items"` -} - -type codeSearchItem struct { - Repository codeSearchRepo `json:"repository"` -} - -type codeSearchRepo struct { - FullName string `json:"full_name"` -} - -// VerifySkillRepos filters a list of repository names to only those that -// actually contain SKILL.md files. It uses the GitHub code search API with -// batched repo: qualifiers. -// -// If a verification call fails (e.g. rate limit), repos in that batch are -// kept rather than silently dropped — we fail open. -func VerifySkillRepos(client RESTClient, host string, repos []string) map[string]bool { - verified := make(map[string]bool) - - for i := 0; i < len(repos); i += verifyBatchSize { - end := i + verifyBatchSize - if end > len(repos) { - end = len(repos) - } - batch := repos[i:end] - - var queryParts []string - queryParts = append(queryParts, "filename:SKILL.md") - for _, r := range batch { - queryParts = append(queryParts, "repo:"+r) - } - query := strings.Join(queryParts, "+") - apiPath := fmt.Sprintf("search/code?q=%s&per_page=%d", query, verifyBatchSize*3) - - var resp codeSearchResponse - if err := client.REST(host, "GET", apiPath, nil, &resp); err != nil { - // Fail open: if we can't verify, assume all repos in the batch are valid - for _, r := range batch { - verified[r] = true - } - continue - } - - for _, item := range resp.Items { - verified[item.Repository.FullName] = true - } - } - - return verified -} diff --git a/internal/skills/discovery/discovery_test.go b/internal/skills/discovery/discovery_test.go index b5fe2410d..5368ad23a 100644 --- a/internal/skills/discovery/discovery_test.go +++ b/internal/skills/discovery/discovery_test.go @@ -1,9 +1,13 @@ package discovery import ( + "net/http" "testing" + "github.com/cli/cli/v2/api" + "github.com/cli/cli/v2/pkg/httpmock" "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" ) func TestInstallName(t *testing.T) { @@ -14,18 +18,18 @@ func TestInstallName(t *testing.T) { }{ { name: "plain skill", - skill: Skill{Name: "git-commit"}, - wantName: "git-commit", + skill: Skill{Name: "code-review"}, + wantName: "code-review", }, { name: "namespaced skill", - skill: Skill{Name: "xlsx-pro", Namespace: "alice"}, - wantName: "alice/xlsx-pro", + skill: Skill{Name: "issue-triage", Namespace: "monalisa"}, + wantName: "monalisa/issue-triage", }, { name: "plugin skill with namespace", - skill: Skill{Name: "code-review", Namespace: "bob", Convention: "plugins"}, - wantName: "bob/code-review", + skill: Skill{Name: "pr-summary", Namespace: "hubot", Convention: "plugins"}, + wantName: "hubot/pr-summary", }, } for _, tt := range tests { @@ -35,48 +39,60 @@ func TestInstallName(t *testing.T) { } } -func TestMatchSkillConventions_PluginNamespace(t *testing.T) { - entry := treeEntry{ - Path: "plugins/bob/skills/code-review/SKILL.md", - Type: "blob", +func TestMatchSkillConventions(t *testing.T) { + tests := []struct { + name string + path string + wantNil bool + wantName string + wantNamespace string + wantConvention string + }{ + { + name: "plugin namespace", + path: "plugins/hubot/skills/pr-summary/SKILL.md", + wantName: "pr-summary", + wantNamespace: "hubot", + wantConvention: "plugins", + }, + { + name: "namespaced skill", + path: "skills/monalisa/issue-triage/SKILL.md", + wantName: "issue-triage", + wantNamespace: "monalisa", + wantConvention: "skills-namespaced", + }, + { + name: "regular skill", + path: "skills/code-review/SKILL.md", + wantName: "code-review", + wantConvention: "skills", + }, + { + name: "non-SKILL.md file", + path: "skills/code-review/README.md", + wantNil: true, + }, } - m := matchSkillConventions(entry) - assert.NotNil(t, m) - assert.Equal(t, "code-review", m.name) - assert.Equal(t, "bob", m.namespace) - assert.Equal(t, "plugins", m.convention) -} - -func TestMatchSkillConventions_NamespacedSkill(t *testing.T) { - entry := treeEntry{ - Path: "skills/alice/xlsx-pro/SKILL.md", - Type: "blob", + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + m := matchSkillConventions(treeEntry{Path: tt.path, Type: "blob"}) + if tt.wantNil { + assert.Nil(t, m) + return + } + require.NotNil(t, m) + assert.Equal(t, tt.wantName, m.name) + assert.Equal(t, tt.wantNamespace, m.namespace) + assert.Equal(t, tt.wantConvention, m.convention) + }) } - m := matchSkillConventions(entry) - assert.NotNil(t, m) - assert.Equal(t, "xlsx-pro", m.name) - assert.Equal(t, "alice", m.namespace) - assert.Equal(t, "skills-namespaced", m.convention) -} - -func TestMatchSkillConventions_RegularSkill(t *testing.T) { - entry := treeEntry{ - Path: "skills/git-commit/SKILL.md", - Type: "blob", - } - m := matchSkillConventions(entry) - assert.NotNil(t, m) - assert.Equal(t, "git-commit", m.name) - assert.Equal(t, "", m.namespace) - assert.Equal(t, "skills", m.convention) } func TestDuplicatePluginSkills_DifferentAuthors(t *testing.T) { - // Simulates a repo with the same skill name under two different plugin authors. - // Previously this caused a collision error; now each gets a distinct namespace. entries := []treeEntry{ - {Path: "plugins/author1/skills/azure-diag/SKILL.md", Type: "blob"}, - {Path: "plugins/author2/skills/azure-diag/SKILL.md", Type: "blob"}, + {Path: "plugins/monalisa/skills/code-review/SKILL.md", Type: "blob"}, + {Path: "plugins/hubot/skills/code-review/SKILL.md", Type: "blob"}, } seen := make(map[string]bool) @@ -90,20 +106,236 @@ func TestDuplicatePluginSkills_DifferentAuthors(t *testing.T) { matches = append(matches, *m) } - assert.Len(t, matches, 2) - assert.Equal(t, "author1", matches[0].namespace) - assert.Equal(t, "author2", matches[1].namespace) + require.Len(t, matches, 2) + assert.Equal(t, "monalisa", matches[0].namespace) + assert.Equal(t, "hubot", matches[1].namespace) + assert.NotEqual(t, + Skill{Name: matches[0].name, Namespace: matches[0].namespace}.InstallName(), + Skill{Name: matches[1].name, Namespace: matches[1].namespace}.InstallName(), + ) +} - // Build skills and verify they have different InstallNames - var skills []Skill - for _, m := range matches { - skills = append(skills, Skill{ - Name: m.name, - Namespace: m.namespace, - Convention: m.convention, +func TestValidateName(t *testing.T) { + tests := []struct { + name string + input string + want bool + }{ + {name: "empty", input: "", want: false}, + {name: "too long", input: string(make([]byte, 65)), want: false}, + {name: "max length", input: "a" + string(make([]byte, 63)), want: false}, // 64 'a's would be valid but []byte gives null bytes + {name: "contains slash", input: "foo/bar", want: false}, + {name: "contains dotdot", input: "foo..bar", want: false}, + {name: "starts with dot", input: ".hidden", want: false}, + {name: "simple name", input: "code-review", want: true}, + {name: "with dots and underscores", input: "octocat_helper.v2", want: true}, + {name: "uppercase allowed", input: "Octocat", want: true}, + {name: "single char", input: "a", want: true}, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + assert.Equal(t, tt.want, validateName(tt.input)) + }) + } +} + +func TestIsSpecCompliant(t *testing.T) { + tests := []struct { + name string + input string + want bool + }{ + {name: "empty", input: "", want: false}, + {name: "consecutive hyphens", input: "code--review", want: false}, + {name: "uppercase rejected", input: "Octocat", want: false}, + {name: "starts with hyphen", input: "-octocat", want: false}, + {name: "ends with hyphen", input: "octocat-", want: false}, + {name: "valid lowercase with hyphens", input: "issue-triage", want: true}, + {name: "valid single char", input: "a", want: true}, + {name: "valid with numbers", input: "copilot4", want: true}, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + assert.Equal(t, tt.want, IsSpecCompliant(tt.input)) + }) + } +} + +func TestResolveRef(t *testing.T) { + tests := []struct { + name string + version string + stubs func(*httpmock.Registry) + wantRef string + wantSHA string + wantErr string + }{ + { + name: "explicit version resolves lightweight tag", + version: "v1.0", + stubs: func(reg *httpmock.Registry) { + reg.Register( + httpmock.REST("GET", "repos/monalisa/octocat-skills/git/ref/tags/v1.0"), + httpmock.JSONResponse(map[string]interface{}{ + "object": map[string]interface{}{"sha": "abc123", "type": "commit"}, + })) + }, + wantRef: "v1.0", + wantSHA: "abc123", + }, + { + name: "explicit version resolves annotated tag", + version: "v2.0", + stubs: func(reg *httpmock.Registry) { + reg.Register( + httpmock.REST("GET", "repos/monalisa/octocat-skills/git/ref/tags/v2.0"), + httpmock.JSONResponse(map[string]interface{}{ + "object": map[string]interface{}{"sha": "tag-obj-sha", "type": "tag"}, + })) + reg.Register( + httpmock.REST("GET", "repos/monalisa/octocat-skills/git/tags/tag-obj-sha"), + httpmock.JSONResponse(map[string]interface{}{ + "object": map[string]interface{}{"sha": "real-commit-sha"}, + })) + }, + wantRef: "v2.0", + wantSHA: "real-commit-sha", + }, + { + name: "explicit version falls back to commit SHA", + version: "deadbeef", + stubs: func(reg *httpmock.Registry) { + reg.Register( + httpmock.REST("GET", "repos/monalisa/octocat-skills/git/ref/tags/deadbeef"), + httpmock.StatusStringResponse(404, "not found")) + reg.Register( + httpmock.REST("GET", "repos/monalisa/octocat-skills/commits/deadbeef"), + httpmock.JSONResponse(map[string]interface{}{"sha": "deadbeef"})) + }, + wantRef: "deadbeef", + wantSHA: "deadbeef", + }, + { + name: "explicit version not found anywhere", + version: "nonexistent", + stubs: func(reg *httpmock.Registry) { + reg.Register( + httpmock.REST("GET", "repos/monalisa/octocat-skills/git/ref/tags/nonexistent"), + httpmock.StatusStringResponse(404, "not found")) + reg.Register( + httpmock.REST("GET", "repos/monalisa/octocat-skills/commits/nonexistent"), + httpmock.StatusStringResponse(404, "not found")) + }, + wantErr: `ref "nonexistent" not found as tag or commit in monalisa/octocat-skills`, + }, + { + name: "no version uses latest release", + stubs: func(reg *httpmock.Registry) { + reg.Register( + httpmock.REST("GET", "repos/monalisa/octocat-skills/releases/latest"), + httpmock.JSONResponse(map[string]interface{}{"tag_name": "v3.0"})) + reg.Register( + httpmock.REST("GET", "repos/monalisa/octocat-skills/git/ref/tags/v3.0"), + httpmock.JSONResponse(map[string]interface{}{ + "object": map[string]interface{}{"sha": "release-sha", "type": "commit"}, + })) + }, + wantRef: "v3.0", + wantSHA: "release-sha", + }, + { + name: "no version falls back to default branch when no releases", + stubs: func(reg *httpmock.Registry) { + reg.Register( + httpmock.REST("GET", "repos/monalisa/octocat-skills/releases/latest"), + httpmock.StatusStringResponse(404, "not found")) + reg.Register( + httpmock.REST("GET", "repos/monalisa/octocat-skills"), + httpmock.JSONResponse(map[string]interface{}{"default_branch": "main"})) + reg.Register( + httpmock.REST("GET", "repos/monalisa/octocat-skills/git/ref/heads/main"), + httpmock.JSONResponse(map[string]interface{}{ + "object": map[string]interface{}{"sha": "branch-sha"}, + })) + }, + wantRef: "main", + wantSHA: "branch-sha", + }, + } + 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}) + + ref, err := ResolveRef(client, "github.com", "monalisa", "octocat-skills", tt.version) + if tt.wantErr != "" { + require.Error(t, err) + assert.Contains(t, err.Error(), tt.wantErr) + return + } + require.NoError(t, err) + assert.Equal(t, tt.wantRef, ref.Ref) + assert.Equal(t, tt.wantSHA, ref.SHA) + }) + } +} + +func TestFetchBlob(t *testing.T) { + tests := []struct { + name string + stubs func(*httpmock.Registry) + wantErr string + want string + }{ + { + name: "decodes base64 content", + stubs: func(reg *httpmock.Registry) { + reg.Register( + httpmock.REST("GET", "repos/monalisa/octocat-skills/git/blobs/abc"), + httpmock.JSONResponse(map[string]interface{}{ + "sha": "abc", "encoding": "base64", "content": "SGVsbG8gV29ybGQ=", + })) + }, + want: "Hello World", + }, + { + name: "rejects non-base64 encoding", + stubs: func(reg *httpmock.Registry) { + reg.Register( + httpmock.REST("GET", "repos/monalisa/octocat-skills/git/blobs/abc"), + httpmock.JSONResponse(map[string]interface{}{ + "sha": "abc", "encoding": "utf-8", "content": "raw", + })) + }, + wantErr: "unexpected blob encoding: utf-8", + }, + { + name: "API error", + stubs: func(reg *httpmock.Registry) { + reg.Register( + httpmock.REST("GET", "repos/monalisa/octocat-skills/git/blobs/abc"), + httpmock.StatusStringResponse(500, "server error")) + }, + wantErr: "could not fetch blob", + }, + } + 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 := FetchBlob(client, "github.com", "monalisa", "octocat-skills", "abc") + 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) }) } - assert.Equal(t, "author1/azure-diag", skills[0].InstallName()) - assert.Equal(t, "author2/azure-diag", skills[1].InstallName()) - assert.NotEqual(t, skills[0].InstallName(), skills[1].InstallName()) } diff --git a/internal/skills/gitclient/gitclient.go b/internal/skills/gitclient/gitclient.go deleted file mode 100644 index 99735db90..000000000 --- a/internal/skills/gitclient/gitclient.go +++ /dev/null @@ -1,149 +0,0 @@ -// Package gitclient provides a shared adapter from the cli/cli git.Client -// (via cmdutil.Factory) to the narrow interfaces used by skills commands. -package gitclient - -import ( - "context" - "os" - "strings" - - "github.com/cli/cli/v2/pkg/cmdutil" -) - -// RootResolver can resolve the git repository root directory. -type RootResolver interface { - ToplevelDir() (string, error) -} - -// RemoteResolver can resolve git remote URLs. -type RemoteResolver interface { - RemoteURL(name string) (string, error) -} - -// Client is the full git operations interface used by skills commands. -type Client interface { - RootResolver - RemoteResolver - GitDir(dir string) error - Remotes() ([]string, error) - CurrentBranch(dir string) (string, error) - IsIgnored(dir, path string) bool -} - -// FactoryClient adapts the cli/cli git.Client to the Client interface. -type FactoryClient struct { - F *cmdutil.Factory -} - -// ToplevelDir returns the root directory of the current git repository. -func (g *FactoryClient) ToplevelDir() (string, error) { - cmd, err := g.F.GitClient.Command(context.Background(), "rev-parse", "--show-toplevel") - if err != nil { - return "", err - } - out, err := cmd.Output() - if err != nil { - return "", err - } - return strings.TrimSpace(string(out)), nil -} - -// RemoteURL returns the URL configured for the named git remote. -func (g *FactoryClient) RemoteURL(name string) (string, error) { - cmd, err := g.F.GitClient.Command(context.Background(), "remote", "get-url", name) - if err != nil { - return "", err - } - out, err := cmd.Output() - if err != nil { - return "", err - } - return strings.TrimSpace(string(out)), nil -} - -// GitDir validates that the given directory is inside a git repository. -func (g *FactoryClient) GitDir(dir string) error { - cmd, err := g.F.GitClient.Command(context.Background(), "-C", dir, "rev-parse", "--git-dir") - if err != nil { - return err - } - _, err = cmd.Output() - return err -} - -// Remotes returns the list of configured git remote names. -func (g *FactoryClient) Remotes() ([]string, error) { - cmd, err := g.F.GitClient.Command(context.Background(), "remote") - if err != nil { - return nil, err - } - out, err := cmd.Output() - if err != nil { - return nil, err - } - return strings.Fields(string(out)), nil -} - -// CurrentBranch returns the current branch name, or "" if HEAD is detached. -func (g *FactoryClient) CurrentBranch(dir string) (string, error) { - cmd, err := g.F.GitClient.Command(context.Background(), "-C", dir, "rev-parse", "--abbrev-ref", "HEAD") - if err != nil { - return "", err - } - out, err := cmd.Output() - if err != nil { - return "", err - } - branch := strings.TrimSpace(string(out)) - if branch == "HEAD" { - return "", nil // detached HEAD - } - return branch, nil -} - -// IsIgnored reports whether the given path is git-ignored in the given directory. -func (g *FactoryClient) IsIgnored(dir, path string) bool { - cmd, err := g.F.GitClient.Command(context.Background(), "-C", dir, "check-ignore", "-q", path) - if err != nil { - return false - } - _, err = cmd.Output() - return err == nil -} - -// ResolveGitRoot returns the git repository root using the provided resolver, -// falling back to the current working directory on error. -func ResolveGitRoot(resolver RootResolver) string { - if resolver == nil { - if cwd, err := os.Getwd(); err == nil { - return cwd - } - return "" - } - root, err := resolver.ToplevelDir() - if err != nil { - if cwd, cwdErr := os.Getwd(); cwdErr == nil { - return cwd - } - return "" - } - return root -} - -// ResolveHomeDir returns the user's home directory, or "" on error. -func ResolveHomeDir() string { - home, err := os.UserHomeDir() - if err != nil { - return "" - } - return home -} - -// TruncateSHA returns the first 8 characters of a SHA, or the full string -// if it is shorter. -func TruncateSHA(sha string) string { - if len(sha) > 8 { - return sha[:8] - } - return sha -} diff --git a/internal/skills/gitclient/gitclient_test.go b/internal/skills/gitclient/gitclient_test.go deleted file mode 100644 index 0b8a2cfff..000000000 --- a/internal/skills/gitclient/gitclient_test.go +++ /dev/null @@ -1,49 +0,0 @@ -package gitclient - -import ( - "fmt" - "testing" - - "github.com/stretchr/testify/assert" -) - -type mockResolver struct { - root string - err error -} - -func (m *mockResolver) ToplevelDir() (string, error) { - if m.err != nil { - return "", m.err - } - return m.root, nil -} - -func TestResolveGitRoot(t *testing.T) { - t.Run("returns root on success", func(t *testing.T) { - got := ResolveGitRoot(&mockResolver{root: "/my/repo"}) - assert.Equal(t, "/my/repo", got) - }) - - t.Run("falls back to cwd on error", func(t *testing.T) { - got := ResolveGitRoot(&mockResolver{err: fmt.Errorf("not a git repo")}) - assert.NotEmpty(t, got) // falls back to cwd - }) - - t.Run("nil resolver falls back to cwd", func(t *testing.T) { - got := ResolveGitRoot(nil) - assert.NotEmpty(t, got) // falls back to cwd - }) -} - -func TestResolveHomeDir(t *testing.T) { - got := ResolveHomeDir() - assert.NotEmpty(t, got) -} - -func TestTruncateSHA(t *testing.T) { - assert.Equal(t, "abcdef12", TruncateSHA("abcdef1234567890")) - assert.Equal(t, "short", TruncateSHA("short")) - assert.Equal(t, "12345678", TruncateSHA("12345678")) - assert.Equal(t, "", TruncateSHA("")) -} diff --git a/internal/skills/hosts/hosts_test.go b/internal/skills/hosts/hosts_test.go deleted file mode 100644 index 78c2a3e9d..000000000 --- a/internal/skills/hosts/hosts_test.go +++ /dev/null @@ -1,113 +0,0 @@ -package hosts - -import ( - "path/filepath" - "testing" - - "github.com/stretchr/testify/assert" - "github.com/stretchr/testify/require" -) - -func TestFindByID(t *testing.T) { - host, err := FindByID("github-copilot") - require.NoError(t, err) - assert.Equal(t, "GitHub Copilot", host.Name) - assert.Equal(t, ".github/skills", host.ProjectDir) -} - -func TestFindByID_Invalid(t *testing.T) { - _, err := FindByID("nonexistent") - assert.Error(t, err) - assert.Contains(t, err.Error(), "unknown host") -} - -func TestValidHostIDs(t *testing.T) { - ids := ValidHostIDs() - assert.Contains(t, ids, "github-copilot") - assert.Contains(t, ids, "claude-code") - assert.Contains(t, ids, "cursor") -} - -func TestHostNames(t *testing.T) { - names := HostNames() - assert.Contains(t, names, "GitHub Copilot") - assert.Contains(t, names, "Claude Code") -} - -func TestInstallDir_Project(t *testing.T) { - host, _ := FindByID("github-copilot") - dir, err := host.InstallDir(ScopeProject, "/tmp/myrepo", "/home/user") - require.NoError(t, err) - assert.Equal(t, filepath.Join("/tmp/myrepo", ".github", "skills"), dir) -} - -func TestInstallDir_User(t *testing.T) { - host, _ := FindByID("github-copilot") - dir, err := host.InstallDir(ScopeUser, "/tmp/myrepo", "/home/user") - require.NoError(t, err) - assert.Equal(t, filepath.Join("/home/user", ".copilot", "skills"), dir) -} - -func TestInstallDir_NoGitRoot(t *testing.T) { - host, _ := FindByID("github-copilot") - _, err := host.InstallDir(ScopeProject, "", "/home/user") - assert.Error(t, err) -} - -func TestRepoNameFromRemote(t *testing.T) { - tests := []struct { - remote string - want string - }{ - {"https://github.com/owner/repo.git", "owner/repo"}, - {"https://github.com/owner/repo", "owner/repo"}, - {"git@github.com:owner/repo.git", "owner/repo"}, - {"git@github.com:owner/repo", "owner/repo"}, - {"ssh://git@github.com/owner/repo.git", "owner/repo"}, - {"ssh://git@github.com/owner/repo", "owner/repo"}, - {"", ""}, - } - for _, tt := range tests { - t.Run(tt.remote, func(t *testing.T) { - assert.Equal(t, tt.want, RepoNameFromRemote(tt.remote)) - }) - } -} - -func TestUniqueProjectDirs(t *testing.T) { - dirs := UniqueProjectDirs() - - // Should contain all known project dirs - assert.Contains(t, dirs, ".github/skills") - assert.Contains(t, dirs, ".claude/skills") - assert.Contains(t, dirs, ".cursor/skills") - assert.Contains(t, dirs, ".agents/skills") - assert.Contains(t, dirs, ".agent/skills") - - // Should deduplicate — gemini and antigravity share .agent/skills - seen := map[string]int{} - for _, d := range dirs { - seen[d]++ - } - for dir, count := range seen { - assert.Equalf(t, 1, count, "directory %q appears %d times, expected 1", dir, count) - } -} - -func TestScopeLabels(t *testing.T) { - t.Run("without repo name", func(t *testing.T) { - labels := ScopeLabels("") - require.Len(t, labels, 2) - assert.Contains(t, labels[0], "Project") - assert.Contains(t, labels[0], "recommended") - assert.Contains(t, labels[1], "Global") - }) - - t.Run("with repo name", func(t *testing.T) { - labels := ScopeLabels("owner/repo") - require.Len(t, labels, 2) - assert.Contains(t, labels[0], "owner/repo") - assert.Contains(t, labels[0], "recommended") - assert.Contains(t, labels[1], "Global") - }) -} diff --git a/internal/skills/installer/installer.go b/internal/skills/installer/installer.go index 4c2a39256..ed2db5074 100644 --- a/internal/skills/installer/installer.go +++ b/internal/skills/installer/installer.go @@ -1,16 +1,19 @@ package installer import ( + "errors" "fmt" "os" "path/filepath" "strings" "sync" + "github.com/cli/cli/v2/api" + "github.com/cli/cli/v2/internal/safepaths" "github.com/cli/cli/v2/internal/skills/discovery" "github.com/cli/cli/v2/internal/skills/frontmatter" - "github.com/cli/cli/v2/internal/skills/hosts" "github.com/cli/cli/v2/internal/skills/lockfile" + "github.com/cli/cli/v2/internal/skills/registry" ) // maxConcurrency limits parallel API requests to avoid rate limiting. @@ -25,12 +28,12 @@ type Options struct { SHA string // resolved commit SHA PinnedRef string // user-supplied --pin value (empty if unpinned) Skills []discovery.Skill - AgentHost *hosts.Host - Scope hosts.Scope + AgentHost *registry.AgentHost + Scope registry.Scope Dir string // explicit target directory (overrides AgentHost+Scope) GitRoot string // git repository root (for project scope) HomeDir string // user home directory (for user scope) - Client discovery.RESTClient + Client *api.Client OnProgress func(done, total int) // called after each skill is installed } @@ -138,8 +141,8 @@ func Install(opts *Options) (*Result, error) { type LocalOptions struct { SourceDir string Skills []discovery.Skill - AgentHost *hosts.Host - Scope hosts.Scope + AgentHost *registry.AgentHost + Scope registry.Scope Dir string GitRoot string HomeDir string @@ -182,7 +185,7 @@ func installLocalSkill(sourceRoot string, skill discovery.Skill, baseDir string) return fmt.Errorf("could not resolve source path: %w", err) } - absSkillDir, err := filepath.Abs(skillDir) + safeSkillDir, err := safepaths.ParseAbsolute(skillDir) if err != nil { return fmt.Errorf("could not resolve target path: %w", err) } @@ -203,20 +206,17 @@ func installLocalSkill(sourceRoot string, skill discovery.Skill, baseDir string) return err } - cleaned := filepath.Clean(relPath) - if cleaned == ".." || strings.HasPrefix(cleaned, ".."+string(filepath.Separator)) { - return nil - } - - destPath := filepath.Join(skillDir, cleaned) - - absDest, err := filepath.Abs(destPath) + // Defensive: filepath.WalkDir cannot produce traversal paths, but we + // guard against it in case the walk input is ever changed. + safeDest, err := safeSkillDir.Join(relPath) if err != nil { + var traversalErr safepaths.PathTraversalError + if errors.As(err, &traversalErr) { + return nil + } return fmt.Errorf("could not resolve destination path: %w", err) } - if !strings.HasPrefix(absDest, absSkillDir+string(filepath.Separator)) && absDest != absSkillDir { - return nil - } + destPath := safeDest.String() if dir := filepath.Dir(destPath); dir != skillDir { if err := os.MkdirAll(dir, 0o755); err != nil { @@ -252,7 +252,7 @@ func installSkill(opts *Options, skill discovery.Skill, baseDir string) error { return fmt.Errorf("could not list skill files: %w", err) } - absSkillDir, err := filepath.Abs(skillDir) + safeSkillDir, err := safepaths.ParseAbsolute(skillDir) if err != nil { return fmt.Errorf("could not resolve skill directory path: %w", err) } @@ -265,20 +265,15 @@ func installSkill(opts *Options, skill discovery.Skill, baseDir string) error { relPath := strings.TrimPrefix(file.Path, skill.Path+"/") - cleaned := filepath.Clean(relPath) - if cleaned == ".." || strings.HasPrefix(cleaned, ".."+string(filepath.Separator)) { - continue - } - - destPath := filepath.Join(skillDir, cleaned) - - absDest, err := filepath.Abs(destPath) + safeDest, err := safeSkillDir.Join(relPath) if err != nil { + var traversalErr safepaths.PathTraversalError + if errors.As(err, &traversalErr) { + continue + } return fmt.Errorf("could not resolve destination path: %w", err) } - if !strings.HasPrefix(absDest, absSkillDir+string(filepath.Separator)) && absDest != absSkillDir { - continue - } + destPath := safeDest.String() if dir := filepath.Dir(destPath); dir != skillDir { if err := os.MkdirAll(dir, 0o755); err != nil { diff --git a/internal/skills/installer/installer_test.go b/internal/skills/installer/installer_test.go new file mode 100644 index 000000000..2f6e09ca8 --- /dev/null +++ b/internal/skills/installer/installer_test.go @@ -0,0 +1,338 @@ +package installer + +import ( + "encoding/base64" + "fmt" + "net/http" + "os" + "path/filepath" + "strings" + "testing" + + "github.com/cli/cli/v2/api" + "github.com/cli/cli/v2/internal/skills/discovery" + "github.com/cli/cli/v2/pkg/httpmock" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +func TestInstallLocalSkill(t *testing.T) { + tests := []struct { + name string + skill discovery.Skill + setup func(t *testing.T, srcDir string) + verify func(t *testing.T, destDir string) + }{ + { + name: "copies files", + skill: discovery.Skill{Name: "code-review", Path: "skills/code-review"}, + setup: func(t *testing.T, srcDir string) { + t.Helper() + skillSrc := filepath.Join(srcDir, "skills", "code-review") + require.NoError(t, os.MkdirAll(skillSrc, 0o755)) + require.NoError(t, os.WriteFile(filepath.Join(skillSrc, "SKILL.md"), []byte("# Code Review"), 0o644)) + require.NoError(t, os.WriteFile(filepath.Join(skillSrc, "prompt.txt"), []byte("review this PR"), 0o644)) + }, + verify: func(t *testing.T, destDir string) { + t.Helper() + content, err := os.ReadFile(filepath.Join(destDir, "code-review", "prompt.txt")) + require.NoError(t, err) + assert.Equal(t, "review this PR", string(content)) + + _, err = os.Stat(filepath.Join(destDir, "code-review", "SKILL.md")) + assert.NoError(t, err) + }, + }, + { + name: "nested directories", + skill: discovery.Skill{Name: "issue-triage", Path: "skills/issue-triage"}, + setup: func(t *testing.T, srcDir string) { + t.Helper() + deep := filepath.Join(srcDir, "skills", "issue-triage", "prompts", "templates") + require.NoError(t, os.MkdirAll(deep, 0o755)) + require.NoError(t, os.WriteFile(filepath.Join(deep, "bug.txt"), []byte("triage bug"), 0o644)) + require.NoError(t, os.WriteFile( + filepath.Join(srcDir, "skills", "issue-triage", "SKILL.md"), []byte("# Issue Triage"), 0o644)) + }, + verify: func(t *testing.T, destDir string) { + t.Helper() + content, err := os.ReadFile(filepath.Join(destDir, "issue-triage", "prompts", "templates", "bug.txt")) + require.NoError(t, err) + assert.Equal(t, "triage bug", string(content)) + }, + }, + { + name: "skips symlinks", + skill: discovery.Skill{Name: "pr-summary", Path: "skills/pr-summary"}, + setup: func(t *testing.T, srcDir string) { + t.Helper() + skillSrc := filepath.Join(srcDir, "skills", "pr-summary") + require.NoError(t, os.MkdirAll(skillSrc, 0o755)) + require.NoError(t, os.WriteFile(filepath.Join(skillSrc, "SKILL.md"), []byte("# PR Summary"), 0o644)) + require.NoError(t, os.WriteFile(filepath.Join(skillSrc, "prompt.txt"), []byte("summarize"), 0o644)) + os.Symlink(filepath.Join(skillSrc, "prompt.txt"), filepath.Join(skillSrc, "link.txt")) + }, + verify: func(t *testing.T, destDir string) { + t.Helper() + _, err := os.Stat(filepath.Join(destDir, "pr-summary", "prompt.txt")) + assert.NoError(t, err) + _, err = os.Stat(filepath.Join(destDir, "pr-summary", "link.txt")) + assert.True(t, os.IsNotExist(err)) + }, + }, + { + name: "injects metadata into SKILL.md", + skill: discovery.Skill{Name: "copilot-helper", Path: "skills/copilot-helper"}, + setup: func(t *testing.T, srcDir string) { + t.Helper() + skillSrc := filepath.Join(srcDir, "skills", "copilot-helper") + require.NoError(t, os.MkdirAll(skillSrc, 0o755)) + require.NoError(t, os.WriteFile(filepath.Join(skillSrc, "SKILL.md"), []byte("# Copilot Helper\nAssists with tasks"), 0o644)) + }, + verify: func(t *testing.T, destDir string) { + t.Helper() + content, err := os.ReadFile(filepath.Join(destDir, "copilot-helper", "SKILL.md")) + require.NoError(t, err) + assert.True(t, strings.Contains(string(content), "local-path"), + "expected SKILL.md to contain local-path metadata, got: %s", string(content)) + }, + }, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + srcDir := t.TempDir() + destDir := t.TempDir() + tt.setup(t, srcDir) + + err := installLocalSkill(srcDir, tt.skill, destDir) + require.NoError(t, err) + tt.verify(t, destDir) + }) + } +} + +func TestInstallSkill(t *testing.T) { + tests := []struct { + name string + skill discovery.Skill + stubs func(*httpmock.Registry) + verify func(t *testing.T, destDir string) + }{ + { + name: "installs files from remote", + skill: discovery.Skill{Name: "code-review", Path: "skills/code-review", TreeSHA: "tree123"}, + stubs: func(reg *httpmock.Registry) { + reg.Register( + httpmock.REST("GET", "repos/monalisa/octocat-skills/git/trees/tree123"), + httpmock.JSONResponse(map[string]interface{}{ + "sha": "tree123", "truncated": false, + "tree": []map[string]interface{}{ + {"path": "SKILL.md", "type": "blob", "sha": "skill-sha", "size": 10}, + {"path": "prompt.txt", "type": "blob", "sha": "prompt-sha", "size": 5}, + }, + })) + reg.Register( + httpmock.REST("GET", "repos/monalisa/octocat-skills/git/blobs/skill-sha"), + httpmock.JSONResponse(map[string]interface{}{ + "sha": "skill-sha", "encoding": "base64", + "content": base64.StdEncoding.EncodeToString([]byte("# Code Review")), + })) + reg.Register( + httpmock.REST("GET", "repos/monalisa/octocat-skills/git/blobs/prompt-sha"), + httpmock.JSONResponse(map[string]interface{}{ + "sha": "prompt-sha", "encoding": "base64", + "content": base64.StdEncoding.EncodeToString([]byte("review this PR")), + })) + }, + verify: func(t *testing.T, destDir string) { + t.Helper() + content, err := os.ReadFile(filepath.Join(destDir, "code-review", "prompt.txt")) + require.NoError(t, err) + assert.Equal(t, "review this PR", string(content)) + + _, err = os.Stat(filepath.Join(destDir, "code-review", "SKILL.md")) + assert.NoError(t, err) + }, + }, + { + name: "injects metadata into SKILL.md", + skill: discovery.Skill{Name: "pr-summary", Path: "skills/pr-summary", TreeSHA: "tree456"}, + stubs: func(reg *httpmock.Registry) { + reg.Register( + httpmock.REST("GET", "repos/monalisa/octocat-skills/git/trees/tree456"), + httpmock.JSONResponse(map[string]interface{}{ + "sha": "tree456", "truncated": false, + "tree": []map[string]interface{}{ + {"path": "SKILL.md", "type": "blob", "sha": "md-sha", "size": 20}, + }, + })) + reg.Register( + httpmock.REST("GET", "repos/monalisa/octocat-skills/git/blobs/md-sha"), + httpmock.JSONResponse(map[string]interface{}{ + "sha": "md-sha", "encoding": "base64", + "content": base64.StdEncoding.EncodeToString([]byte("# PR Summary\nSummarize pull requests")), + })) + }, + verify: func(t *testing.T, destDir string) { + 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") + }, + }, + { + name: "skips path traversal from malicious tree", + skill: discovery.Skill{Name: "code-review", Path: "skills/code-review", TreeSHA: "tree123"}, + stubs: func(reg *httpmock.Registry) { + reg.Register( + httpmock.REST("GET", "repos/monalisa/octocat-skills/git/trees/tree123"), + httpmock.JSONResponse(map[string]interface{}{ + "sha": "tree123", "truncated": false, + "tree": []map[string]interface{}{ + {"path": "SKILL.md", "type": "blob", "sha": "safe-sha", "size": 10}, + {"path": "../../etc/passwd", "type": "blob", "sha": "evil-sha", "size": 100}, + }, + })) + reg.Register( + httpmock.REST("GET", "repos/monalisa/octocat-skills/git/blobs/safe-sha"), + httpmock.JSONResponse(map[string]interface{}{ + "sha": "safe-sha", "encoding": "base64", + "content": base64.StdEncoding.EncodeToString([]byte("# Safe Skill")), + })) + reg.Register( + httpmock.REST("GET", "repos/monalisa/octocat-skills/git/blobs/evil-sha"), + httpmock.JSONResponse(map[string]interface{}{ + "sha": "evil-sha", "encoding": "base64", + "content": base64.StdEncoding.EncodeToString([]byte("malicious content")), + })) + }, + verify: func(t *testing.T, destDir string) { + t.Helper() + _, err := os.Stat(filepath.Join(destDir, "code-review", "SKILL.md")) + assert.NoError(t, err) + + _, err = os.Stat(filepath.Join(destDir, "..", "etc", "passwd")) + assert.True(t, os.IsNotExist(err), "traversal path should not be written") + }, + }, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + destDir := t.TempDir() + reg := &httpmock.Registry{} + defer reg.Verify(t) + tt.stubs(reg) + client := api.NewClientFromHTTP(&http.Client{Transport: reg}) + opts := &Options{ + Host: "github.com", + Owner: "monalisa", + Repo: "octocat-skills", + Ref: "v1.0", + SHA: "commit123", + Client: client, + } + + err := installSkill(opts, tt.skill, destDir) + require.NoError(t, err) + tt.verify(t, destDir) + }) + } +} + +func stubTreeAndBlob(reg *httpmock.Registry, treeSHA string) { + reg.Register( + httpmock.REST("GET", fmt.Sprintf("repos/monalisa/octocat-skills/git/trees/%s", treeSHA)), + httpmock.JSONResponse(map[string]interface{}{ + "sha": treeSHA, "truncated": false, + "tree": []map[string]interface{}{ + {"path": "SKILL.md", "type": "blob", "sha": treeSHA + "-blob", "size": 10}, + }, + })) + reg.Register( + httpmock.REST("GET", fmt.Sprintf("repos/monalisa/octocat-skills/git/blobs/%s-blob", treeSHA)), + httpmock.JSONResponse(map[string]interface{}{ + "sha": treeSHA + "-blob", "encoding": "base64", + "content": base64.StdEncoding.EncodeToString([]byte("# Skill")), + })) +} + +func TestInstall(t *testing.T) { + tests := []struct { + name string + skills []discovery.Skill + stubs func(*httpmock.Registry) + wantInstalled []string + wantErr string + }{ + { + name: "single skill", + skills: []discovery.Skill{ + {Name: "code-review", Path: "skills/code-review", TreeSHA: "tree-cr"}, + }, + stubs: func(reg *httpmock.Registry) { stubTreeAndBlob(reg, "tree-cr") }, + wantInstalled: []string{"code-review"}, + }, + { + name: "multiple skills concurrently", + skills: []discovery.Skill{ + {Name: "code-review", Path: "skills/code-review", TreeSHA: "tree-cr"}, + {Name: "issue-triage", Path: "skills/issue-triage", TreeSHA: "tree-it"}, + }, + stubs: func(reg *httpmock.Registry) { + stubTreeAndBlob(reg, "tree-cr") + stubTreeAndBlob(reg, "tree-it") + }, + wantInstalled: []string{"code-review", "issue-triage"}, + }, + { + name: "no dir or agent host", + skills: []discovery.Skill{{Name: "code-review"}}, + stubs: func(reg *httpmock.Registry) {}, + wantErr: "either Dir or AgentHost must be specified", + }, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + t.Setenv("HOME", t.TempDir()) + + destDir := t.TempDir() + reg := &httpmock.Registry{} + defer reg.Verify(t) + tt.stubs(reg) + client := api.NewClientFromHTTP(&http.Client{Transport: reg}) + + opts := &Options{ + Host: "github.com", + Owner: "monalisa", + Repo: "octocat-skills", + Ref: "v1.0", + SHA: "commit123", + Client: client, + Skills: tt.skills, + Dir: destDir, + } + if tt.wantErr != "" { + opts.Dir = "" + } + + result, err := Install(opts) + if tt.wantErr != "" { + require.Error(t, err) + assert.Contains(t, err.Error(), tt.wantErr) + return + } + require.NoError(t, err) + assert.ElementsMatch(t, tt.wantInstalled, result.Installed) + assert.Equal(t, destDir, result.Dir) + + homeDir, _ := os.UserHomeDir() + lockPath := filepath.Join(homeDir, ".agents", ".skill-lock.json") + lockData, err := os.ReadFile(lockPath) + require.NoError(t, err, "lockfile should have been written") + for _, name := range tt.wantInstalled { + assert.Contains(t, string(lockData), name) + } + }) + } +} diff --git a/internal/skills/lockfile/lockfile.go b/internal/skills/lockfile/lockfile.go index ad5fd4d4b..5761d24cf 100644 --- a/internal/skills/lockfile/lockfile.go +++ b/internal/skills/lockfile/lockfile.go @@ -15,8 +15,8 @@ const ( lockFile = ".skill-lock.json" ) -// Entry represents a single installed skill in the lock file. -type Entry struct { +// entry represents a single installed skill in the lock file. +type entry struct { Source string `json:"source"` SourceType string `json:"sourceType"` SourceURL string `json:"sourceUrl"` @@ -27,15 +27,15 @@ type Entry struct { PinnedRef string `json:"pinnedRef,omitempty"` } -// File is the top-level structure of .skill-lock.json. -type File struct { +// file is the top-level structure of .skill-lock.json. +type file struct { Version int `json:"version"` - Skills map[string]Entry `json:"skills"` + Skills map[string]entry `json:"skills"` Dismissed map[string]bool `json:"dismissed,omitempty"` } -// Path returns the absolute path to the lock file. -func Path() (string, error) { +// lockfilePath returns the absolute path to the lock file. +func lockfilePath() (string, error) { home, err := os.UserHomeDir() if err != nil { return "", err @@ -43,10 +43,10 @@ func Path() (string, error) { return filepath.Join(home, agentsDir, lockFile), nil } -// Read loads the lock file, returning an empty file if it doesn't exist +// read loads the lock file, returning an empty file if it doesn't exist // or if it's an incompatible version. -func Read() (*File, error) { - lockPath, err := Path() +func read() (*file, error) { + lockPath, err := lockfilePath() if err != nil { return newFile(), nil //nolint:nilerr // graceful: no home dir means fresh state } @@ -59,7 +59,7 @@ func Read() (*File, error) { return nil, fmt.Errorf("could not read lock file: %w", err) } - var f File + var f file if err := json.Unmarshal(data, &f); err != nil { return newFile(), nil //nolint:nilerr // graceful: corrupt file means fresh state } @@ -71,9 +71,9 @@ func Read() (*File, error) { return &f, nil } -// Write persists the lock file to disk. -func Write(f *File) error { - lockPath, err := Path() +// write persists the lock file to disk. +func write(f *file) error { + lockPath, err := lockfilePath() if err != nil { return err } @@ -97,7 +97,7 @@ func RecordInstall(skillName, owner, repo, skillPath, treeSHA, pinnedRef string) unlock := acquireLock() defer unlock() - f, err := Read() + f, err := read() if err != nil { return err } @@ -110,7 +110,7 @@ func RecordInstall(skillName, owner, repo, skillPath, treeSHA, pinnedRef string) installedAt = existing.InstalledAt } - f.Skills[skillName] = Entry{ + f.Skills[skillName] = entry{ Source: owner + "/" + repo, SourceType: "github", SourceURL: "https://github.com/" + owner + "/" + repo + ".git", @@ -121,13 +121,13 @@ func RecordInstall(skillName, owner, repo, skillPath, treeSHA, pinnedRef string) PinnedRef: pinnedRef, } - return Write(f) + return write(f) } -func newFile() *File { - return &File{ +func newFile() *file { + return &file{ Version: lockVersion, - Skills: make(map[string]Entry), + Skills: make(map[string]entry), } } @@ -135,7 +135,7 @@ func newFile() *File { // Returns an unlock function. If locking fails after retries, it proceeds // unlocked rather than blocking the user indefinitely. func acquireLock() (unlock func()) { - lockPath, pathErr := Path() + lockPath, pathErr := lockfilePath() if pathErr != nil { return func() {} } diff --git a/internal/skills/lockfile/lockfile_test.go b/internal/skills/lockfile/lockfile_test.go new file mode 100644 index 000000000..b53d6aafc --- /dev/null +++ b/internal/skills/lockfile/lockfile_test.go @@ -0,0 +1,193 @@ +package lockfile + +import ( + "encoding/json" + "os" + "path/filepath" + "testing" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +// setupHome redirects HOME to a temp dir and returns the expected lockfile path. +func setupHome(t *testing.T) string { + t.Helper() + home := t.TempDir() + t.Setenv("HOME", home) + return filepath.Join(home, agentsDir, lockFile) +} + +func TestRecordInstall(t *testing.T) { + tests := []struct { + name string + setup func(t *testing.T) // optional pre-existing state + skill string + owner string + repo string + skillPath string + treeSHA string + pinnedRef string + verify func(t *testing.T, lockPath string) + }{ + { + name: "fresh install creates lockfile", + skill: "code-review", + owner: "monalisa", + repo: "octocat-skills", + skillPath: "skills/code-review/SKILL.md", + treeSHA: "abc123", + verify: func(t *testing.T, lockPath string) { + t.Helper() + f := readLockfile(t, lockPath) + require.Contains(t, f.Skills, "code-review") + e := f.Skills["code-review"] + assert.Equal(t, "monalisa/octocat-skills", e.Source) + assert.Equal(t, "github", e.SourceType) + assert.Equal(t, "https://github.com/monalisa/octocat-skills.git", e.SourceURL) + assert.Equal(t, "skills/code-review/SKILL.md", e.SkillPath) + assert.Equal(t, "abc123", e.SkillFolderHash) + assert.NotEmpty(t, e.InstalledAt) + assert.NotEmpty(t, e.UpdatedAt) + assert.Empty(t, e.PinnedRef) + }, + }, + { + name: "install with pinned ref", + skill: "pr-summary", + owner: "hubot", + repo: "skills-repo", + skillPath: "skills/pr-summary/SKILL.md", + treeSHA: "def456", + pinnedRef: "v1.0.0", + verify: func(t *testing.T, lockPath string) { + t.Helper() + f := readLockfile(t, lockPath) + assert.Equal(t, "v1.0.0", f.Skills["pr-summary"].PinnedRef) + }, + }, + { + name: "update preserves InstalledAt and updates treeSHA", + setup: func(t *testing.T) { + t.Helper() + require.NoError(t, RecordInstall("code-review", "monalisa", "octocat-skills", "skills/code-review/SKILL.md", "old-sha", "")) + }, + skill: "code-review", + owner: "monalisa", + repo: "octocat-skills", + skillPath: "skills/code-review/SKILL.md", + treeSHA: "new-sha", + verify: func(t *testing.T, lockPath string) { + t.Helper() + f := readLockfile(t, lockPath) + e := f.Skills["code-review"] + assert.Equal(t, "new-sha", e.SkillFolderHash, "treeSHA should be updated") + // InstalledAt should be preserved (not empty proves it wasn't clobbered) + assert.NotEmpty(t, e.InstalledAt, "InstalledAt should be preserved from first install") + }, + }, + { + name: "multiple skills coexist", + setup: func(t *testing.T) { + t.Helper() + require.NoError(t, RecordInstall("code-review", "monalisa", "octocat-skills", "skills/code-review/SKILL.md", "sha1", "")) + }, + skill: "issue-triage", + owner: "monalisa", + repo: "octocat-skills", + skillPath: "skills/issue-triage/SKILL.md", + treeSHA: "sha2", + verify: func(t *testing.T, lockPath string) { + t.Helper() + f := readLockfile(t, lockPath) + assert.Contains(t, f.Skills, "code-review") + assert.Contains(t, f.Skills, "issue-triage") + }, + }, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + lockPath := setupHome(t) + if tt.setup != nil { + tt.setup(t) + } + + err := RecordInstall(tt.skill, tt.owner, tt.repo, tt.skillPath, tt.treeSHA, tt.pinnedRef) + require.NoError(t, err) + tt.verify(t, lockPath) + }) + } +} + +func TestRead(t *testing.T) { + tests := []struct { + name string + setup func(t *testing.T, lockPath string) + wantSkill bool + }{ + { + name: "missing file returns fresh state", + setup: func(t *testing.T, lockPath string) {}, + }, + { + name: "corrupt JSON returns fresh state", + setup: func(t *testing.T, lockPath string) { + t.Helper() + require.NoError(t, os.MkdirAll(filepath.Dir(lockPath), 0o755)) + require.NoError(t, os.WriteFile(lockPath, []byte("{invalid json"), 0o644)) + }, + }, + { + name: "wrong version returns fresh state", + setup: func(t *testing.T, lockPath string) { + t.Helper() + require.NoError(t, os.MkdirAll(filepath.Dir(lockPath), 0o755)) + data, _ := json.Marshal(file{Version: 999, Skills: map[string]entry{"x": {}}}) + require.NoError(t, os.WriteFile(lockPath, data, 0o644)) + }, + }, + { + name: "valid lockfile", + setup: func(t *testing.T, lockPath string) { + t.Helper() + require.NoError(t, os.MkdirAll(filepath.Dir(lockPath), 0o755)) + f := &file{ + Version: lockVersion, + Skills: map[string]entry{ + "code-review": {Source: "monalisa/octocat-skills", SourceType: "github"}, + }, + } + data, err := json.MarshalIndent(f, "", " ") + require.NoError(t, err) + require.NoError(t, os.WriteFile(lockPath, data, 0o644)) + }, + wantSkill: true, + }, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + lockPath := setupHome(t) + tt.setup(t, lockPath) + + loaded, err := read() + require.NoError(t, err) + assert.Equal(t, lockVersion, loaded.Version) + + if tt.wantSkill { + assert.Contains(t, loaded.Skills, "code-review") + } else { + assert.Empty(t, loaded.Skills) + } + }) + } +} + +// readLockfile is a test helper that reads and parses the lockfile from disk. +func readLockfile(t *testing.T, path string) *file { + t.Helper() + data, err := os.ReadFile(path) + require.NoError(t, err, "lockfile should exist at %s", path) + var f file + require.NoError(t, json.Unmarshal(data, &f)) + return &f +} diff --git a/internal/skills/hosts/hosts.go b/internal/skills/registry/registry.go similarity index 72% rename from internal/skills/hosts/hosts.go rename to internal/skills/registry/registry.go index bee20b0f0..ecaaaa48d 100644 --- a/internal/skills/hosts/hosts.go +++ b/internal/skills/registry/registry.go @@ -1,16 +1,17 @@ -package hosts +package registry import ( "fmt" "path/filepath" + "strings" "github.com/cli/cli/v2/git" "github.com/cli/cli/v2/internal/ghrepo" ) -// Host represents an AI agent that can use skills. -type Host struct { - // ID is the canonical identifier for this host. +// AgentHost represents an AI agent that can use skills. +type AgentHost struct { + // ID is the canonical identifier for this agent host. ID string // Name is the human-readable display name. Name string @@ -28,8 +29,8 @@ const ( ScopeUser Scope = "user" ) -// Registry contains all known agent hosts. -var Registry = []Host{ +// Agents contains all known agent hosts. +var Agents = []AgentHost{ { ID: "github-copilot", Name: "GitHub Copilot", @@ -68,52 +69,45 @@ var Registry = []Host{ }, } -// FindByID returns the host with the given ID, or an error if not found. -func FindByID(id string) (*Host, error) { - for i := range Registry { - if Registry[i].ID == id { - return &Registry[i], nil +// FindByID returns the agent host with the given ID, or an error if not found. +func FindByID(id string) (*AgentHost, error) { + for i := range Agents { + if Agents[i].ID == id { + return &Agents[i], nil } } - return nil, fmt.Errorf("unknown host %q, valid hosts: %s", id, ValidHostIDs()) + return nil, fmt.Errorf("unknown agent %q, valid agents: %s", id, ValidAgentIDs()) } -// ValidHostIDs returns a comma-separated list of valid host IDs. -func ValidHostIDs() string { - ids := "" - for i, h := range Registry { - if i > 0 { - ids += ", " - } - ids += h.ID - } - return ids +// ValidAgentIDs returns a comma-separated list of valid agent IDs. +func ValidAgentIDs() string { + return strings.Join(AgentIDs(), ", ") } -// HostIDs returns the IDs of all known hosts as a slice. -func HostIDs() []string { - ids := make([]string, len(Registry)) - for i, h := range Registry { +// AgentIDs returns the IDs of all known agents as a slice. +func AgentIDs() []string { + ids := make([]string, len(Agents)) + for i, h := range Agents { ids[i] = h.ID } return ids } -// HostNames returns the display names of all hosts for prompting. -func HostNames() []string { - names := make([]string, len(Registry)) - for i, h := range Registry { +// AgentNames returns the display names of all agents for prompting. +func AgentNames() []string { + names := make([]string, len(Agents)) + for i, h := range Agents { names[i] = h.Name } return names } // UniqueProjectDirs returns the deduplicated set of project-scope skill -// directories from the Registry, preserving insertion order. +// directories from the Agents list, preserving insertion order. func UniqueProjectDirs() []string { seen := map[string]bool{} var dirs []string - for _, h := range Registry { + for _, h := range Agents { if !seen[h.ProjectDir] { seen[h.ProjectDir] = true dirs = append(dirs, h.ProjectDir) @@ -122,12 +116,12 @@ func UniqueProjectDirs() []string { return dirs } -// InstallDir resolves the absolute installation directory for a host and scope. +// InstallDir resolves the absolute installation directory for an agent host and scope. // For project scope, it uses the provided git root directory so that skills are // installed at the top level regardless of which subdirectory the user is in. // Returns an error when gitRoot is empty (not in a git repository). // For user scope, it uses the home directory. -func (h *Host) InstallDir(scope Scope, gitRoot, homeDir string) (string, error) { +func (h *AgentHost) InstallDir(scope Scope, gitRoot, homeDir string) (string, error) { switch scope { case ScopeProject: if gitRoot == "" { diff --git a/internal/skills/registry/registry_test.go b/internal/skills/registry/registry_test.go new file mode 100644 index 000000000..f37c35e96 --- /dev/null +++ b/internal/skills/registry/registry_test.go @@ -0,0 +1,153 @@ +package registry + +import ( + "path/filepath" + "testing" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +func TestFindByID(t *testing.T) { + tests := []struct { + name string + id string + wantName string + wantErr string + }{ + {name: "github-copilot", id: "github-copilot", wantName: "GitHub Copilot"}, + {name: "claude-code", id: "claude-code", wantName: "Claude Code"}, + {name: "cursor", id: "cursor", wantName: "Cursor"}, + {name: "codex", id: "codex", wantName: "Codex"}, + {name: "gemini", id: "gemini", wantName: "Gemini CLI"}, + {name: "antigravity", id: "antigravity", wantName: "Antigravity"}, + {name: "unknown agent", id: "nonexistent", wantErr: "unknown agent"}, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + host, err := FindByID(tt.id) + if tt.wantErr != "" { + require.Error(t, err) + assert.Contains(t, err.Error(), tt.wantErr) + return + } + require.NoError(t, err) + assert.Equal(t, tt.wantName, host.Name) + }) + } +} + +func TestInstallDir(t *testing.T) { + host, err := FindByID("github-copilot") + require.NoError(t, err) + + tests := []struct { + name string + scope Scope + gitRoot string + homeDir string + wantDir string + wantErr bool + }{ + { + name: "project scope", + scope: ScopeProject, + gitRoot: "/tmp/monalisa-repo", + homeDir: "/home/monalisa", + wantDir: filepath.Join("/tmp/monalisa-repo", ".github", "skills"), + }, + { + name: "user scope", + scope: ScopeUser, + gitRoot: "/tmp/monalisa-repo", + homeDir: "/home/monalisa", + wantDir: filepath.Join("/home/monalisa", ".copilot", "skills"), + }, + { + name: "project scope without git root", + scope: ScopeProject, + gitRoot: "", + homeDir: "/home/monalisa", + wantErr: true, + }, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + dir, err := host.InstallDir(tt.scope, tt.gitRoot, tt.homeDir) + if tt.wantErr { + assert.Error(t, err) + return + } + require.NoError(t, err) + assert.Equal(t, tt.wantDir, dir) + }) + } +} + +func TestRepoNameFromRemote(t *testing.T) { + tests := []struct { + remote string + want string + }{ + {"https://github.com/monalisa/octocat-skills.git", "monalisa/octocat-skills"}, + {"https://github.com/monalisa/octocat-skills", "monalisa/octocat-skills"}, + {"git@github.com:monalisa/octocat-skills.git", "monalisa/octocat-skills"}, + {"git@github.com:monalisa/octocat-skills", "monalisa/octocat-skills"}, + {"ssh://git@github.com/monalisa/octocat-skills.git", "monalisa/octocat-skills"}, + {"ssh://git@github.com/monalisa/octocat-skills", "monalisa/octocat-skills"}, + {"", ""}, + } + for _, tt := range tests { + t.Run(tt.remote, func(t *testing.T) { + assert.Equal(t, tt.want, RepoNameFromRemote(tt.remote)) + }) + } +} + +func TestUniqueProjectDirs(t *testing.T) { + dirs := UniqueProjectDirs() + require.NotEmpty(t, dirs) + + // Should deduplicate — e.g. gemini and antigravity share .agent/skills + seen := map[string]int{} + for _, d := range dirs { + seen[d]++ + } + for dir, count := range seen { + assert.Equalf(t, 1, count, "directory %q appears %d times, expected 1", dir, count) + } +} + +func TestScopeLabels(t *testing.T) { + tests := []struct { + name string + repoName string + wantFirst []string + wantSecond []string + }{ + { + name: "without repo name", + repoName: "", + wantFirst: []string{"Project", "recommended"}, + wantSecond: []string{"Global"}, + }, + { + name: "with repo name", + repoName: "monalisa/octocat-skills", + wantFirst: []string{"monalisa/octocat-skills", "recommended"}, + wantSecond: []string{"Global"}, + }, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + labels := ScopeLabels(tt.repoName) + require.Len(t, labels, 2) + for _, s := range tt.wantFirst { + assert.Contains(t, labels[0], s) + } + for _, s := range tt.wantSecond { + assert.Contains(t, labels[1], s) + } + }) + } +} diff --git a/pkg/cmd/skills/install/install.go b/pkg/cmd/skills/install/install.go index 38613800d..eac2e4a00 100644 --- a/pkg/cmd/skills/install/install.go +++ b/pkg/cmd/skills/install/install.go @@ -1,6 +1,7 @@ package install import ( + "context" "errors" "fmt" "io" @@ -12,14 +13,14 @@ import ( "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/ghrepo" "github.com/cli/cli/v2/internal/prompter" - "github.com/cli/cli/v2/internal/skills" "github.com/cli/cli/v2/internal/skills/discovery" "github.com/cli/cli/v2/internal/skills/frontmatter" - "github.com/cli/cli/v2/internal/skills/gitclient" - "github.com/cli/cli/v2/internal/skills/hosts" "github.com/cli/cli/v2/internal/skills/installer" + "github.com/cli/cli/v2/internal/skills/registry" "github.com/cli/cli/v2/internal/text" "github.com/cli/cli/v2/pkg/cmdutil" "github.com/cli/cli/v2/pkg/iostreams" @@ -40,7 +41,8 @@ type installOptions struct { IO *iostreams.IOStreams HttpClient func() (*http.Client, error) Prompter prompter.Prompter - GitClient installGitClient + GitClient *git.Client + Remotes func() (ghContext.Remotes, error) // Arguments SkillSource string // owner/repo or local path @@ -61,18 +63,13 @@ type installOptions struct { version string } -// installGitClient is the git interface needed by the install command. -type installGitClient interface { - gitclient.RootResolver - gitclient.RemoteResolver -} - // NewCmdInstall creates the "skills install" command. func NewCmdInstall(f *cmdutil.Factory, runF func(*installOptions) error) *cobra.Command { opts := &installOptions{ IO: f.IOStreams, Prompter: f.Prompter, - GitClient: &gitclient.FactoryClient{F: f}, + GitClient: f.GitClient, + Remotes: f.Remotes, HttpClient: f.HttpClient, } @@ -188,7 +185,7 @@ func NewCmdInstall(f *cmdutil.Factory, runF func(*installOptions) error) *cobra. } if opts.Agent != "" { - if _, err := hosts.FindByID(opts.Agent); err != nil { + if _, err := registry.FindByID(opts.Agent); err != nil { return cmdutil.FlagErrorf("invalid value for --agent: %s", err) } } @@ -204,9 +201,9 @@ func NewCmdInstall(f *cmdutil.Factory, runF func(*installOptions) error) *cobra. }, } - cmd.Flags().StringVar(&opts.Agent, "agent", "", fmt.Sprintf("target agent (%s)", hosts.ValidHostIDs())) + cmd.Flags().StringVar(&opts.Agent, "agent", "", fmt.Sprintf("target agent (%s)", registry.ValidAgentIDs())) _ = cmd.RegisterFlagCompletionFunc("agent", func(_ *cobra.Command, _ []string, _ string) ([]string, cobra.ShellCompDirective) { - return hosts.HostIDs(), cobra.ShellCompDirectiveNoFileComp + return registry.AgentIDs(), cobra.ShellCompDirectiveNoFileComp }) cmdutil.StringEnumFlag(cmd, &opts.Scope, "scope", "", "project", []string{"project", "user"}, "Installation scope") cmd.Flags().StringVar(&opts.Pin, "pin", "", "pin to a specific git tag or commit SHA") @@ -287,12 +284,12 @@ func installRun(opts *installOptions) error { return err } - gitRoot := gitclient.ResolveGitRoot(opts.GitClient) - homeDir := gitclient.ResolveHomeDir() + gitRoot := resolveGitRoot(opts.GitClient) + homeDir := resolveHomeDir() source = ghrepo.FullName(opts.repo) type hostPlan struct { - host *hosts.Host + host *registry.AgentHost skills []discovery.Skill } var plans []hostPlan @@ -426,11 +423,11 @@ func runLocalInstall(opts *installOptions) error { return err } - gitRoot := gitclient.ResolveGitRoot(opts.GitClient) - homeDir := gitclient.ResolveHomeDir() + gitRoot := resolveGitRoot(opts.GitClient) + homeDir := resolveHomeDir() type hostPlan struct { - host *hosts.Host + host *registry.AgentHost skills []discovery.Skill } var plans []hostPlan @@ -534,18 +531,18 @@ func cutLast(s, sep string) (before, after string, found bool) { return s, "", false } -func resolveVersion(opts *installOptions, client discovery.RESTClient, hostname string) (*discovery.ResolvedRef, error) { +func resolveVersion(opts *installOptions, client *api.Client, hostname string) (*discovery.ResolvedRef, error) { opts.IO.StartProgressIndicatorWithLabel("Resolving version") resolved, err := discovery.ResolveRef(client, hostname, opts.repo.RepoOwner(), opts.repo.RepoName(), opts.version) opts.IO.StopProgressIndicator() if err != nil { return nil, fmt.Errorf("could not resolve version: %w", err) } - fmt.Fprintf(opts.IO.ErrOut, "Using ref %s (%s)\n", resolved.Ref, gitclient.TruncateSHA(resolved.SHA)) + fmt.Fprintf(opts.IO.ErrOut, "Using ref %s (%s)\n", resolved.Ref, git.ShortSHA(resolved.SHA)) return resolved, nil } -func discoverSkills(opts *installOptions, client discovery.RESTClient, hostname string, resolved *discovery.ResolvedRef) ([]discovery.Skill, error) { +func discoverSkills(opts *installOptions, client *api.Client, hostname string, resolved *discovery.ResolvedRef) ([]discovery.Skill, error) { opts.IO.StartProgressIndicatorWithLabel("Discovering skills") skills, err := discovery.DiscoverSkills(client, hostname, opts.repo.RepoOwner(), opts.repo.RepoName(), resolved.SHA) opts.IO.StopProgressIndicator() @@ -755,7 +752,7 @@ func matchSelectedSkills(skills []discovery.Skill, selected []string) ([]discove // collisionError checks for name collisions and returns an error with // guidance on how to install skills individually. func collisionError(ss []discovery.Skill, sourceHint string) error { - collisions := skills.FindNameCollisions(ss) + collisions := discovery.FindNameCollisions(ss) if len(collisions) == 0 { return nil } @@ -764,28 +761,28 @@ func collisionError(ss []discovery.Skill, sourceHint string) error { %s Install these skills individually using the full name: gh skills install %s namespace/skill-name - `, skills.FormatCollisions(collisions), sourceHint)) + `, discovery.FormatCollisions(collisions), sourceHint)) } -func resolveHosts(opts *installOptions, canPrompt bool) ([]*hosts.Host, error) { +func resolveHosts(opts *installOptions, canPrompt bool) ([]*registry.AgentHost, error) { if opts.Agent != "" { - h, err := hosts.FindByID(opts.Agent) + h, err := registry.FindByID(opts.Agent) if err != nil { return nil, err } - return []*hosts.Host{h}, nil + return []*registry.AgentHost{h}, nil } if !canPrompt { - h, err := hosts.FindByID("github-copilot") + h, err := registry.FindByID("github-copilot") if err != nil { return nil, err } - return []*hosts.Host{h}, nil + return []*registry.AgentHost{h}, nil } fmt.Fprintln(opts.IO.ErrOut) - names := hosts.HostNames() + names := registry.AgentNames() indices, err := opts.Prompter.MultiSelect("Select target agent(s):", []string{names[0]}, names) if err != nil { return nil, err @@ -795,41 +792,43 @@ func resolveHosts(opts *installOptions, canPrompt bool) ([]*hosts.Host, error) { return nil, fmt.Errorf("must select at least one target agent") } - selected := make([]*hosts.Host, len(indices)) + selected := make([]*registry.AgentHost, len(indices)) for i, idx := range indices { - selected[i] = &hosts.Registry[idx] + selected[i] = ®istry.Agents[idx] } return selected, nil } -func resolveScope(opts *installOptions, canPrompt bool) (hosts.Scope, error) { +func resolveScope(opts *installOptions, canPrompt bool) (registry.Scope, error) { if opts.Dir != "" { - return hosts.Scope(opts.Scope), nil + return registry.Scope(opts.Scope), nil } if opts.ScopeChanged || !canPrompt { - return hosts.Scope(opts.Scope), nil + return registry.Scope(opts.Scope), nil } var repoName string - if remote, err := opts.GitClient.RemoteURL("origin"); err == nil { - repoName = hosts.RepoNameFromRemote(remote) + if opts.Remotes != nil { + if remotes, err := opts.Remotes(); err == nil && len(remotes) > 0 { + repoName = ghrepo.FullName(remotes[0].Repo) + } } - idx, err := opts.Prompter.Select("Installation scope:", "", hosts.ScopeLabels(repoName)) + idx, err := opts.Prompter.Select("Installation scope:", "", registry.ScopeLabels(repoName)) if err != nil { return "", err } if idx == 0 { - return hosts.ScopeProject, nil + return registry.ScopeProject, nil } - return hosts.ScopeUser, nil + return registry.ScopeUser, nil } func truncateDescription(s string, maxWidth int) string { return text.Truncate(maxWidth, text.RemoveExcessiveWhitespace(s)) } -func checkOverwrite(opts *installOptions, skills []discovery.Skill, host *hosts.Host, scope hosts.Scope, gitRoot, homeDir string, canPrompt bool) ([]discovery.Skill, error) { +func checkOverwrite(opts *installOptions, skills []discovery.Skill, host *registry.AgentHost, scope registry.Scope, gitRoot, homeDir string, canPrompt bool) ([]discovery.Skill, error) { targetDir := opts.Dir if targetDir == "" { var err error @@ -991,3 +990,28 @@ func printReviewHint(w io.Writer, cs *iostreams.ColorScheme, repo string, skillN } fmt.Fprintln(w) } + +func resolveGitRoot(gc *git.Client) string { + if gc == nil { + if cwd, err := os.Getwd(); err == nil { + return cwd + } + return "" + } + root, err := gc.ToplevelDir(context.Background()) + if err != nil { + if cwd, err := os.Getwd(); err == nil { + return cwd + } + return "" + } + return root +} + +func resolveHomeDir() string { + home, err := os.UserHomeDir() + if err != nil { + return "" + } + return home +} diff --git a/pkg/cmd/skills/install/install_test.go b/pkg/cmd/skills/install/install_test.go index f53fd2267..658815b63 100644 --- a/pkg/cmd/skills/install/install_test.go +++ b/pkg/cmd/skills/install/install_test.go @@ -13,8 +13,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/skills/gitclient" - "github.com/cli/cli/v2/internal/skills/hosts" + "github.com/cli/cli/v2/internal/skills/registry" "github.com/cli/cli/v2/pkg/cmdutil" "github.com/cli/cli/v2/pkg/httpmock" "github.com/cli/cli/v2/pkg/iostreams" @@ -23,27 +22,6 @@ import ( "github.com/stretchr/testify/require" ) -// mockGitClient implements installGitClient for testing. -type mockGitClient struct { - root string - remote string - err error -} - -func (m *mockGitClient) ToplevelDir() (string, error) { - if m.err != nil { - return "", m.err - } - return m.root, nil -} - -func (m *mockGitClient) RemoteURL(_ string) (string, error) { - if m.err != nil { - return "", m.err - } - return m.remote, nil -} - func TestNewCmdInstall_Help(t *testing.T) { ios, _, _, _ := iostreams.Test() f := &cmdutil.Factory{ @@ -184,7 +162,7 @@ func TestInstallRun_NonInteractive_NoRepo(t *testing.T) { opts := &installOptions{ IO: ios, - GitClient: &mockGitClient{root: "/tmp", remote: ""}, + GitClient: &git.Client{RepoDir: t.TempDir()}, } err := installRun(opts) @@ -368,11 +346,6 @@ func TestResolveHosts_NoneSelected(t *testing.T) { assert.Error(t, err) } -func TestTruncateSHA(t *testing.T) { - assert.Equal(t, "abc123de", gitclient.TruncateSHA("abc123def456")) - assert.Equal(t, "short", gitclient.TruncateSHA("short")) -} - func TestTruncateDescription(t *testing.T) { tests := []struct { name string @@ -457,7 +430,7 @@ func TestRunLocalInstall_NonInteractive(t *testing.T) { Agent: "github-copilot", Scope: "project", Dir: targetDir, - GitClient: &mockGitClient{root: t.TempDir(), remote: ""}, + GitClient: &git.Client{RepoDir: t.TempDir()}, } err := installRun(opts) @@ -489,7 +462,7 @@ func TestRunLocalInstall_SingleSkillDir(t *testing.T) { Agent: "github-copilot", Scope: "project", Dir: targetDir, - GitClient: &mockGitClient{root: t.TempDir(), remote: ""}, + GitClient: &git.Client{RepoDir: t.TempDir()}, } err := installRun(opts) @@ -577,7 +550,7 @@ func TestResolveScope_ExplicitFlag(t *testing.T) { IO: ios, Scope: "user", ScopeChanged: true, - GitClient: &mockGitClient{root: "/tmp", remote: ""}, + GitClient: &git.Client{RepoDir: t.TempDir()}, } scope, err := resolveScope(opts, true) require.NoError(t, err) @@ -590,7 +563,7 @@ func TestResolveScope_DirBypasses(t *testing.T) { IO: ios, Dir: "/tmp/custom", Scope: "project", - GitClient: &mockGitClient{root: "/tmp", remote: ""}, + GitClient: &git.Client{RepoDir: t.TempDir()}, } scope, err := resolveScope(opts, true) require.NoError(t, err) @@ -601,10 +574,10 @@ func TestCheckOverwrite_NoExisting(t *testing.T) { ios, _, _, _ := iostreams.Test() targetDir := t.TempDir() skills := []discovery.Skill{{Name: "new-skill"}} - host := &hosts.Host{ID: "test", ProjectDir: "skills"} + host := ®istry.AgentHost{ID: "test", ProjectDir: "skills"} opts := &installOptions{IO: ios, Dir: targetDir} - got, err := checkOverwrite(opts, skills, host, hosts.ScopeProject, "/tmp", "/home", false) + got, err := checkOverwrite(opts, skills, host, registry.ScopeProject, "/tmp", "/home", false) require.NoError(t, err) assert.Len(t, got, 1) } @@ -615,10 +588,10 @@ func TestCheckOverwrite_ExistingWithForce(t *testing.T) { ios, _, _, _ := iostreams.Test() skills := []discovery.Skill{{Name: "existing-skill"}} - host := &hosts.Host{ID: "test", ProjectDir: "skills"} + host := ®istry.AgentHost{ID: "test", ProjectDir: "skills"} opts := &installOptions{IO: ios, Dir: targetDir, Force: true} - got, err := checkOverwrite(opts, skills, host, hosts.ScopeProject, "/tmp", "/home", false) + got, err := checkOverwrite(opts, skills, host, registry.ScopeProject, "/tmp", "/home", false) require.NoError(t, err) assert.Len(t, got, 1) } @@ -629,10 +602,10 @@ func TestCheckOverwrite_ExistingNonInteractive(t *testing.T) { ios, _, _, _ := iostreams.Test() skills := []discovery.Skill{{Name: "existing-skill"}} - host := &hosts.Host{ID: "test", ProjectDir: "skills"} + host := ®istry.AgentHost{ID: "test", ProjectDir: "skills"} opts := &installOptions{IO: ios, Dir: targetDir} - _, err := checkOverwrite(opts, skills, host, hosts.ScopeProject, "/tmp", "/home", false) + _, err := checkOverwrite(opts, skills, host, registry.ScopeProject, "/tmp", "/home", false) assert.Error(t, err) assert.Contains(t, err.Error(), "already installed") } @@ -755,7 +728,7 @@ func TestInstallRun_RemoteInstall(t *testing.T) { HttpClient: func() (*http.Client, error) { return &http.Client{Transport: reg}, nil }, - GitClient: &mockGitClient{root: t.TempDir(), remote: ""}, + GitClient: &git.Client{RepoDir: t.TempDir()}, SkillSource: "owner/repo", SkillName: "test-skill", Agent: "github-copilot", @@ -880,7 +853,7 @@ func TestRunLocalInstall_NamespacedSkills(t *testing.T) { Agent: "github-copilot", Scope: "project", Dir: targetDir, - GitClient: &mockGitClient{root: t.TempDir(), remote: ""}, + GitClient: &git.Client{RepoDir: t.TempDir()}, } err := installRun(opts) @@ -908,10 +881,10 @@ func TestCheckOverwrite_NamespacedSkill(t *testing.T) { {Name: "xlsx-pro", Namespace: "alice"}, {Name: "xlsx-pro", Namespace: "bob"}, } - host := &hosts.Host{ID: "test", ProjectDir: "skills"} + host := ®istry.AgentHost{ID: "test", ProjectDir: "skills"} opts := &installOptions{IO: ios, Dir: targetDir, Force: true} - got, err := checkOverwrite(opts, skills, host, hosts.ScopeProject, "/tmp", "/home", false) + got, err := checkOverwrite(opts, skills, host, registry.ScopeProject, "/tmp", "/home", false) require.NoError(t, err) assert.Len(t, got, 2, "both skills should be installable (force mode)") } diff --git a/pkg/cmd/skills/preview/preview.go b/pkg/cmd/skills/preview/preview.go new file mode 100644 index 000000000..9541f4230 --- /dev/null +++ b/pkg/cmd/skills/preview/preview.go @@ -0,0 +1,382 @@ +package preview + +import ( + "fmt" + "io" + "net/http" + "sort" + "strings" + + "github.com/MakeNowJust/heredoc" + "github.com/cli/cli/v2/api" + "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/pkg/cmdutil" + "github.com/cli/cli/v2/pkg/iostreams" + "github.com/cli/cli/v2/pkg/markdown" + "github.com/spf13/cobra" +) + +type previewOptions struct { + IO *iostreams.IOStreams + HttpClient func() (*http.Client, error) + Prompter prompter.Prompter + Executable func() string + + RepoArg string + SkillName string + + repo ghrepo.Interface +} + +// NewCmdPreview creates the "skills preview" command. +func NewCmdPreview(f *cmdutil.Factory, runF func(*previewOptions) error) *cobra.Command { + opts := &previewOptions{ + IO: f.IOStreams, + HttpClient: f.HttpClient, + Prompter: f.Prompter, + Executable: f.Executable, + } + + cmd := &cobra.Command{ + Use: "preview []", + Short: "Preview a skill from a GitHub repository", + Long: heredoc.Doc(` + Render a skill's SKILL.md content in the terminal. This fetches the + skill file from the repository and displays it using the configured + pager, without installing anything. + + A file tree is shown first, followed by the rendered SKILL.md content. + When running interactively and the skill contains additional files + (scripts, references, etc.), a file picker lets you browse them + individually. + + When run with only a repository argument, lists available skills and + prompts for selection. + `), + Example: heredoc.Doc(` + # Preview a specific skill + $ gh skills preview github/awesome-copilot code-review + + # Browse and preview interactively + $ gh skills preview github/awesome-copilot + `), + Aliases: []string{"show"}, + Args: cobra.RangeArgs(1, 2), + RunE: func(c *cobra.Command, args []string) error { + opts.RepoArg = args[0] + if len(args) == 2 { + opts.SkillName = args[1] + } + + repo, err := ghrepo.FromFullName(opts.RepoArg) + if err != nil { + return err + } + opts.repo = repo + + if runF != nil { + return runF(opts) + } + return previewRun(opts) + }, + } + + return cmd +} + +func previewRun(opts *previewOptions) error { + cs := opts.IO.ColorScheme() + + repo := opts.repo + owner := repo.RepoOwner() + repoName := repo.RepoName() + hostname := repo.RepoHost() + + httpClient, err := opts.HttpClient() + if err != nil { + return err + } + apiClient := api.NewClientFromHTTP(httpClient) + + opts.IO.StartProgressIndicatorWithLabel(fmt.Sprintf("Resolving %s/%s", owner, repoName)) + resolved, err := discovery.ResolveRef(apiClient, hostname, owner, repoName, "") + opts.IO.StopProgressIndicator() + if err != nil { + return fmt.Errorf("could not resolve version: %w", err) + } + + opts.IO.StartProgressIndicatorWithLabel("Discovering skills") + skills, err := discovery.DiscoverSkills(apiClient, hostname, owner, repoName, resolved.SHA) + opts.IO.StopProgressIndicator() + if err != nil { + return err + } + + sort.Slice(skills, func(i, j int) bool { + return skills[i].DisplayName() < skills[j].DisplayName() + }) + + skill, err := selectSkill(opts, skills) + if err != nil { + return err + } + + opts.IO.StartProgressIndicatorWithLabel("Fetching skill content") + var files []discovery.SkillFile + if skill.TreeSHA != "" { + files, err = discovery.ListSkillFiles(apiClient, hostname, owner, repoName, skill.TreeSHA) + if err != nil { + fmt.Fprintf(opts.IO.ErrOut, "warning: could not list skill files: %v\n", err) + files = nil + } + } + content, err := discovery.FetchBlob(apiClient, hostname, owner, repoName, skill.BlobSHA) + opts.IO.StopProgressIndicator() + if err != nil { + return err + } + + parsed, parseErr := frontmatter.Parse(content) + if parseErr == nil { + content = parsed.Body + } + + rendered, err := markdown.Render(content, + markdown.WithTheme(opts.IO.TerminalTheme()), + markdown.WithWrap(opts.IO.TerminalWidth()), + markdown.WithoutIndentation()) + if err != nil { + rendered = content + } + + // Collect extra files (everything that isn't SKILL.md) + var extraFiles []discovery.SkillFile + for _, f := range files { + if f.Path != "SKILL.md" { + extraFiles = append(extraFiles, f) + } + } + + canPrompt := opts.IO.CanPrompt() + + // 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) + } + + // Interactive with multiple files: show tree, then file picker + return renderInteractive(opts, cs, skill, files, rendered, extraFiles, apiClient, hostname, owner, repoName) +} + +// 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 { + + opts.IO.DetectTerminalTheme() + if err := opts.IO.StartPager(); err != nil { + fmt.Fprintf(opts.IO.ErrOut, "starting pager failed: %v\n", err) + } + defer opts.IO.StopPager() + + out := opts.IO.Out + + if len(files) > 0 { + fmt.Fprintf(out, "%s\n", cs.Bold(skill.DisplayName()+"/")) + renderFileTree(out, cs, files) + fmt.Fprintln(out) + } + + fmt.Fprintf(out, "%s\n\n", cs.Bold("── SKILL.md ──")) + fmt.Fprint(out, rendered) + + const maxFiles = 20 + const maxTotalBytes = 512 * 1024 + fetched := 0 + totalBytes := 0 + for _, f := range extraFiles { + if fetched >= maxFiles { + fmt.Fprintf(out, "\n%s\n", cs.Gray(fmt.Sprintf("(skipped remaining files — showing first %d)", maxFiles))) + break + } + if totalBytes+f.Size > maxTotalBytes && fetched > 0 { + fmt.Fprintf(out, "\n%s\n", cs.Gray("(skipped remaining files — size limit reached)")) + break + } + fileContent, fetchErr := discovery.FetchBlob(apiClient, hostname, owner, repo, f.SHA) + if fetchErr != nil { + fmt.Fprintf(out, "\n%s\n\n%s\n", cs.Bold("── "+f.Path+" ──"), cs.Gray("(could not fetch file)")) + continue + } + fetched++ + totalBytes += len(fileContent) + fmt.Fprintf(out, "\n%s\n\n", cs.Bold("── "+f.Path+" ──")) + fmt.Fprint(out, fileContent) + if !strings.HasSuffix(fileContent, "\n") { + 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 { + + // Show the file tree to stderr so it persists above the prompt + fmt.Fprintf(opts.IO.ErrOut, "\n%s\n", cs.Bold(skill.DisplayName()+"/")) + renderFileTree(opts.IO.ErrOut, cs, files) + fmt.Fprintln(opts.IO.ErrOut) + + // Build choices: SKILL.md first, then extra files + choices := make([]string, 0, len(extraFiles)+1) + choices = append(choices, "SKILL.md") + for _, f := range extraFiles { + choices = append(choices, f.Path) + } + + // Save original stdout — StopPager closes IO.Out, so we need to + // restore a working writer before each StartPager call. + originalOut := opts.IO.Out + + for { + // Restore original Out before each pager cycle. StartPager replaces + // IO.Out with a pipe; StopPager closes that pipe but does not + // restore the original. The original writer remains valid. + opts.IO.Out = originalOut + + 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 + } + + var content string + + if idx == 0 { + content = renderedSkillMD + } else { + selectedFile := extraFiles[idx-1] + + // Fetch on demand — don't hold blob data in memory + fileContent, fetchErr := discovery.FetchBlob(apiClient, hostname, owner, repo, selectedFile.SHA) + if fetchErr != nil { + fmt.Fprintf(opts.IO.ErrOut, "%s could not fetch %s: %v\n", cs.Red("!"), selectedFile.Path, fetchErr) + continue + } + content = fileContent + if !strings.HasSuffix(content, "\n") { + content += "\n" + } + } + + if err := opts.IO.StartPager(); err != nil { + fmt.Fprintf(opts.IO.ErrOut, "starting pager failed: %v\n", err) + } + fmt.Fprint(opts.IO.Out, content) + opts.IO.StopPager() + } +} + +func selectSkill(opts *previewOptions, skills []discovery.Skill) (discovery.Skill, error) { + if opts.SkillName != "" { + for _, s := range skills { + if s.DisplayName() == opts.SkillName || s.Name == opts.SkillName { + return s, nil + } + } + return discovery.Skill{}, fmt.Errorf("skill %q not found in %s", opts.SkillName, ghrepo.FullName(opts.repo)) + } + + if !opts.IO.CanPrompt() { + return discovery.Skill{}, fmt.Errorf("must specify a skill name when not running interactively") + } + + choices := make([]string, len(skills)) + for i, s := range skills { + choices[i] = s.DisplayName() + } + + idx, err := opts.Prompter.Select("Select a skill to preview:", "", choices) + if err != nil { + return discovery.Skill{}, err + } + + return skills[idx], nil +} + +// treeNode represents a file or directory in the tree for rendering. +type treeNode struct { + name string + children []*treeNode + isDir bool +} + +// renderFileTree prints a tree of skill files using box-drawing characters. +func renderFileTree(w io.Writer, cs *iostreams.ColorScheme, files []discovery.SkillFile) { + root := buildTree(files) + printTree(w, cs, root.children, "") +} + +// buildTree constructs a tree structure from flat file paths. +func buildTree(files []discovery.SkillFile) *treeNode { + root := &treeNode{isDir: true} + for _, f := range files { + parts := strings.Split(f.Path, "/") + current := root + for i, part := range parts { + isLast := i == len(parts)-1 + found := false + for _, child := range current.children { + if child.name == part { + current = child + found = true + break + } + } + if !found { + node := &treeNode{name: part, isDir: !isLast} + current.children = append(current.children, node) + current = node + } + } + } + sortTree(root) + return root +} + +func sortTree(node *treeNode) { + sort.Slice(node.children, func(i, j int) bool { + if node.children[i].isDir != node.children[j].isDir { + return node.children[i].isDir + } + return node.children[i].name < node.children[j].name + }) + for _, child := range node.children { + if child.isDir { + sortTree(child) + } + } +} + +func printTree(w io.Writer, cs *iostreams.ColorScheme, nodes []*treeNode, indent string) { + for i, node := range nodes { + isLast := i == len(nodes)-1 + connector := "├── " + childIndent := "│ " + if isLast { + connector = "└── " + childIndent = " " + } + if node.isDir { + fmt.Fprintf(w, "%s%s%s\n", indent, cs.Gray(connector), cs.Bold(node.name+"/")) + printTree(w, cs, node.children, indent+cs.Gray(childIndent)) + } else { + fmt.Fprintf(w, "%s%s%s\n", indent, cs.Gray(connector), node.name) + } + } +} diff --git a/pkg/cmd/skills/preview/preview_test.go b/pkg/cmd/skills/preview/preview_test.go new file mode 100644 index 000000000..b77455828 --- /dev/null +++ b/pkg/cmd/skills/preview/preview_test.go @@ -0,0 +1,466 @@ +package preview + +import ( + "encoding/base64" + "fmt" + "net/http" + "testing" + + "github.com/cli/cli/v2/internal/ghrepo" + "github.com/cli/cli/v2/internal/prompter" + "github.com/cli/cli/v2/pkg/cmdutil" + "github.com/cli/cli/v2/pkg/httpmock" + "github.com/cli/cli/v2/pkg/iostreams" + "github.com/google/shlex" + "github.com/stretchr/testify/assert" +) + +func TestNewCmdPreview(t *testing.T) { + tests := []struct { + name string + input string + wantRepo string + wantSkillName string + wantErr bool + }{ + { + name: "repo and skill", + input: "github/awesome-copilot my-skill", + wantRepo: "github/awesome-copilot", + wantSkillName: "my-skill", + }, + { + name: "repo only", + input: "github/awesome-copilot", + wantRepo: "github/awesome-copilot", + }, + { + name: "no args", + input: "", + wantErr: true, + }, + { + name: "too many args", + input: "a b c", + wantErr: true, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + ios, _, _, _ := iostreams.Test() + f := &cmdutil.Factory{ + IOStreams: ios, + Prompter: &prompter.PrompterMock{}, + } + + var gotOpts *previewOptions + cmd := NewCmdPreview(f, func(opts *previewOptions) error { + gotOpts = opts + return nil + }) + + args, _ := shlex.Split(tt.input) + cmd.SetArgs(args) + cmd.SetOut(&discardWriter{}) + cmd.SetErr(&discardWriter{}) + err := cmd.Execute() + + if tt.wantErr { + assert.Error(t, err) + return + } + + assert.NoError(t, err) + assert.Equal(t, tt.wantRepo, gotOpts.RepoArg) + assert.Equal(t, tt.wantSkillName, gotOpts.SkillName) + }) + } +} + +func TestNewCmdPreview_Alias(t *testing.T) { + ios, _, _, _ := iostreams.Test() + f := &cmdutil.Factory{IOStreams: ios, Prompter: &prompter.PrompterMock{}} + cmd := NewCmdPreview(f, func(_ *previewOptions) error { return nil }) + assert.Contains(t, cmd.Aliases, "show") +} + +func TestPreviewRun(t *testing.T) { + skillContent := "---\nname: my-skill\ndescription: A test skill\n---\n# My Skill\n\nThis is the skill content." + encodedContent := base64.StdEncoding.EncodeToString([]byte(skillContent)) + + tests := []struct { + name string + opts *previewOptions + tty bool + httpStubs func(*httpmock.Registry) + wantStdout string + wantErr string + }{ + { + name: "preview specific skill", + tty: true, + opts: &previewOptions{ + repo: ghrepo.New("github", "awesome-copilot"), + SkillName: "my-skill", + }, + httpStubs: func(reg *httpmock.Registry) { + reg.Register( + httpmock.REST("GET", "repos/github/awesome-copilot/releases/latest"), + httpmock.StringResponse(`{"tag_name": "v1.0.0"}`), + ) + reg.Register( + httpmock.REST("GET", "repos/github/awesome-copilot/git/ref/tags/v1.0.0"), + httpmock.StringResponse(`{"object": {"sha": "abc123", "type": "commit"}}`), + ) + reg.Register( + httpmock.REST("GET", "repos/github/awesome-copilot/git/trees/abc123"), + httpmock.StringResponse(`{ + "sha": "abc123", + "truncated": false, + "tree": [ + {"path": "skills", "type": "tree", "sha": "tree1"}, + {"path": "skills/my-skill", "type": "tree", "sha": "treeSHA"}, + {"path": "skills/my-skill/SKILL.md", "type": "blob", "sha": "blob123"} + ] + }`), + ) + reg.Register( + httpmock.REST("GET", "repos/github/awesome-copilot/git/trees/treeSHA"), + httpmock.StringResponse(`{ + "tree": [ + {"path": "SKILL.md", "type": "blob", "sha": "blob123", "size": 50} + ] + }`), + ) + reg.Register( + httpmock.REST("GET", "repos/github/awesome-copilot/git/blobs/blob123"), + httpmock.StringResponse(`{"sha": "blob123", "content": "`+encodedContent+`", "encoding": "base64"}`), + ) + }, + wantStdout: "My Skill", + }, + { + name: "preview with display name match", + tty: true, + opts: &previewOptions{ + repo: ghrepo.New("owner", "repo"), + SkillName: "ns/my-skill", + }, + httpStubs: func(reg *httpmock.Registry) { + 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", "type": "tree", "sha": "tree1"}, + {"path": "skills/ns", "type": "tree", "sha": "tree-ns"}, + {"path": "skills/ns/my-skill", "type": "tree", "sha": "treeSHA2"}, + {"path": "skills/ns/my-skill/SKILL.md", "type": "blob", "sha": "blob456"} + ] + }`), + ) + reg.Register( + httpmock.REST("GET", "repos/owner/repo/git/trees/treeSHA2"), + httpmock.StringResponse(`{ + "tree": [ + {"path": "SKILL.md", "type": "blob", "sha": "blob456", "size": 50} + ] + }`), + ) + reg.Register( + httpmock.REST("GET", "repos/owner/repo/git/blobs/blob456"), + httpmock.StringResponse(`{"sha": "blob456", "content": "`+encodedContent+`", "encoding": "base64"}`), + ) + }, + wantStdout: "My Skill", + }, + { + name: "skill not found", + tty: true, + opts: &previewOptions{ + repo: ghrepo.New("owner", "repo"), + SkillName: "nonexistent", + }, + httpStubs: func(reg *httpmock.Registry) { + 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": "tree2"}, + {"path": "skills/my-skill/SKILL.md", "type": "blob", "sha": "blob123"} + ] + }`), + ) + }, + wantErr: `skill "nonexistent" not found in owner/repo`, + }, + { + name: "no skill name non-interactive errors", + tty: false, + opts: &previewOptions{ + repo: ghrepo.New("owner", "repo"), + }, + httpStubs: func(reg *httpmock.Registry) { + 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": "tree2"}, + {"path": "skills/my-skill/SKILL.md", "type": "blob", "sha": "blob123"} + ] + }`), + ) + }, + wantErr: "must specify a skill name when not running interactively", + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + reg := &httpmock.Registry{} + defer reg.Verify(t) + if tt.httpStubs != nil { + tt.httpStubs(reg) + } + tt.opts.HttpClient = func() (*http.Client, error) { + return &http.Client{Transport: reg}, nil + } + + ios, _, stdout, _ := iostreams.Test() + ios.SetStdoutTTY(tt.tty) + ios.SetStdinTTY(tt.tty) + tt.opts.IO = ios + + tt.opts.Prompter = &prompter.PrompterMock{} + + err := previewRun(tt.opts) + + if tt.wantErr != "" { + assert.EqualError(t, err, tt.wantErr) + return + } + + assert.NoError(t, err) + if tt.wantStdout != "" { + assert.Contains(t, stdout.String(), tt.wantStdout) + } + }) + } +} + +func TestPreviewRun_Interactive(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"}`), + ) + + ios, _, stdout, _ := iostreams.Test() + ios.SetStdoutTTY(true) + ios.SetStdinTTY(true) + + pm := &prompter.PrompterMock{ + SelectFunc: func(prompt string, defaultValue string, options []string) (int, error) { + assert.Equal(t, "Select a skill to preview:", prompt) + assert.Equal(t, []string{"alpha", "beta"}, options) + return 1, nil // select "beta" + }, + } + + opts := &previewOptions{ + IO: ios, + HttpClient: func() (*http.Client, error) { return &http.Client{Transport: reg}, nil }, + Prompter: pm, + repo: ghrepo.New("owner", "repo"), + } + + err := previewRun(opts) + assert.NoError(t, err) + assert.Contains(t, stdout.String(), "Selected Skill") +} + +func TestPreviewRun_ShowsFileTree(t *testing.T) { + skillContent := "---\nname: my-skill\ndescription: test\n---\n# My Skill\nBody." + encodedContent := base64.StdEncoding.EncodeToString([]byte(skillContent)) + + scriptContent := "#!/bin/bash\necho hello" + encodedScript := base64.StdEncoding.EncodeToString([]byte(scriptContent)) + + makeReg := func() *httpmock.Registry { + reg := &httpmock.Registry{} + 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"}, + {"path": "skills/my-skill/scripts", "type": "tree", "sha": "treeScripts"}, + {"path": "skills/my-skill/scripts/run.sh", "type": "blob", "sha": "blobScript"} + ] + }`), + ) + reg.Register( + httpmock.REST("GET", "repos/owner/repo/git/trees/treeSHA"), + httpmock.StringResponse(`{ + "tree": [ + {"path": "SKILL.md", "type": "blob", "sha": "blobSKILL", "size": 50}, + {"path": "scripts", "type": "tree", "sha": "treeScripts"}, + {"path": "scripts/run.sh", "type": "blob", "sha": "blobScript", "size": 20} + ] + }`), + ) + reg.Register( + httpmock.REST("GET", "repos/owner/repo/git/blobs/blobSKILL"), + httpmock.StringResponse(`{"sha": "blobSKILL", "content": "`+encodedContent+`", "encoding": "base64"}`), + ) + reg.Register( + httpmock.REST("GET", "repos/owner/repo/git/blobs/blobScript"), + httpmock.StringResponse(`{"sha": "blobScript", "content": "`+encodedScript+`", "encoding": "base64"}`), + ) + return reg + } + + t.Run("interactive file picker", func(t *testing.T) { + reg := makeReg() + defer reg.Verify(t) + ios, _, stdout, _ := iostreams.Test() + ios.SetStdoutTTY(true) + ios.SetStdinTTY(true) + ios.SetColorEnabled(false) + + selectCalls := 0 + pm := &prompter.PrompterMock{ + SelectFunc: func(prompt string, defaultValue string, options []string) (int, error) { + selectCalls++ + if selectCalls == 1 { + // Options: ["SKILL.md", "scripts/run.sh"] + assert.Equal(t, "SKILL.md", options[0]) + assert.Equal(t, "scripts/run.sh", options[1]) + // Select "scripts/run.sh" + return 1, nil + } + // Simulate Esc/Ctrl-C to exit + return 0, fmt.Errorf("user cancelled") + }, + } + + opts := &previewOptions{ + IO: ios, + HttpClient: func() (*http.Client, error) { return &http.Client{Transport: reg}, nil }, + Prompter: pm, + repo: ghrepo.New("owner", "repo"), + SkillName: "my-skill", + } + + err := previewRun(opts) + assert.NoError(t, err) + + out := stdout.String() + assert.Contains(t, out, "echo hello") + assert.Equal(t, 2, selectCalls) + }) + + t.Run("non-interactive dumps all files", func(t *testing.T) { + reg := makeReg() + defer reg.Verify(t) + ios, _, stdout, _ := iostreams.Test() + ios.SetStdoutTTY(false) + ios.SetStdinTTY(false) + ios.SetColorEnabled(false) + + opts := &previewOptions{ + IO: ios, + HttpClient: func() (*http.Client, error) { return &http.Client{Transport: reg}, nil }, + Prompter: &prompter.PrompterMock{}, + repo: ghrepo.New("owner", "repo"), + SkillName: "my-skill", + } + + err := previewRun(opts) + assert.NoError(t, err) + + out := stdout.String() + assert.Contains(t, out, "my-skill/") + assert.Contains(t, out, "My Skill") + assert.Contains(t, out, "scripts/run.sh") + assert.Contains(t, out, "echo hello") + }) +} + +// discardWriter is a no-op writer for suppressing cobra output in tests. +type discardWriter struct{} + +func (d *discardWriter) Write(p []byte) (int, error) { return len(p), nil } diff --git a/pkg/cmd/skills/publish/publish.go b/pkg/cmd/skills/publish/publish.go new file mode 100644 index 000000000..9a9200131 --- /dev/null +++ b/pkg/cmd/skills/publish/publish.go @@ -0,0 +1,1246 @@ +package publish + +import ( + "bytes" + "context" + "encoding/json" + "fmt" + "net/http" + "os" + "path/filepath" + "regexp" + "sort" + "strings" + + "github.com/MakeNowJust/heredoc" + "github.com/cli/cli/v2/api" + "github.com/cli/cli/v2/git" + giturl "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/pkg/cmdutil" + "github.com/cli/cli/v2/pkg/iostreams" + "github.com/spf13/cobra" +) + +// publishOptions holds all dependencies and user-provided flags for the publish command. +type publishOptions struct { + IO *iostreams.IOStreams + HttpClient func() (*http.Client, error) + Config func() (gh.Config, error) + Prompter prompter.Prompter + GitClient *git.Client + + // Arguments + Dir string // directory to validate (default: cwd) + + // Flags + Fix bool // --fix flag: auto-fix issues where possible + Plugins bool // --plugins flag: generate .claude-plugin/ manifest + DryRun bool // --dry-run flag: validate only, don't publish + Tag string // --tag flag: release tag to create + + // Testing overrides + client *api.Client // injectable for tests; nil means use factory HttpClient + host string // API host (e.g. "github.com"); resolved from config in production +} + +// publishDiagnostic is a single validation finding. +type publishDiagnostic struct { + skill string // empty for repo-level issues + severity string // "error", "warning", "fixed", or "info" + message string +} + +// repoTopicsResponse is the response from the repo topics API. +type repoTopicsResponse struct { + Names []string `json:"names"` +} + +// tagEntry is a single tag from the tags list API. +type tagEntry struct { + Name string `json:"name"` +} + +// rulesetsResponse is a single ruleset from the rulesets API. +type rulesetsResponse struct { + ID int `json:"id"` + Name string `json:"name"` + Target string `json:"target"` + Enforcement string `json:"enforcement"` +} + +// securityAnalysis represents the security_and_analysis field from the repo API. +type securityAnalysis struct { + AdvancedSecurity *securityFeature `json:"advanced_security"` + SecretScanning *securityFeature `json:"secret_scanning"` + SecretScanningPushProtection *securityFeature `json:"secret_scanning_push_protection"` +} + +type securityFeature struct { + Status string `json:"status"` +} + +// repoSecurityResponse is the subset of repo API we need for security checks. +type repoSecurityResponse struct { + SecurityAndAnalysis *securityAnalysis `json:"security_and_analysis"` +} + +// NewCmdPublish creates the "skills publish" command. +func NewCmdPublish(f *cmdutil.Factory, runF func(*publishOptions) error) *cobra.Command { + opts := &publishOptions{ + IO: f.IOStreams, + HttpClient: f.HttpClient, + Config: f.Config, + Prompter: f.Prompter, + GitClient: f.GitClient, + } + + cmd := &cobra.Command{ + Use: "publish []", + Short: "Validate and publish skills to a GitHub repository", + Long: heredoc.Doc(` + Validate a local repository's skills against the Agent Skills specification + and publish them by creating a GitHub release. + + Validation checks include: + + - Skills follow the skills/*/SKILL.md directory convention + - Skill names match the strict agentskills.io naming rules + - Each skill name matches its directory name + - Required frontmatter fields (name, description) are present + - allowed-tools is a string, not an array + - Install metadata (metadata.github-*) is stripped if present + + After validation passes, publish will interactively guide you through: + + - Adding the "agent-skills" topic to the repository + - Choosing a version tag (semver recommended) + - Creating a GitHub release with auto-generated notes + + Use --dry-run to validate without publishing. + Use --tag to publish non-interactively with a specific tag. + Use --fix to automatically strip install metadata from committed files. + Use --plugins to generate a .claude-plugin/plugin.json manifest for + Claude Code plugin discovery. + `), + Example: heredoc.Doc(` + # Validate and publish interactively + $ gh skills publish + + # Publish with a specific tag (non-interactive) + $ gh skills publish --tag v1.0.0 + + # Validate only (no publish) + $ gh skills publish --dry-run + + # Validate and strip install metadata + $ gh skills publish --fix + + # Generate Claude Code plugin manifest + $ gh skills publish --plugins + `), + Aliases: []string{"validate"}, + Args: cobra.MaximumNArgs(1), + RunE: func(cmd *cobra.Command, args []string) error { + if len(args) == 1 { + opts.Dir = args[0] + } + if runF != nil { + return runF(opts) + } + return publishRun(opts) + }, + } + + cmd.Flags().BoolVar(&opts.Fix, "fix", false, "Auto-fix issues where possible (e.g. strip install metadata)") + cmd.Flags().BoolVar(&opts.Plugins, "plugins", false, "Generate .claude-plugin/ manifest for Claude Code plugin discovery") + cmd.Flags().BoolVar(&opts.DryRun, "dry-run", false, "Validate without publishing") + cmd.Flags().StringVar(&opts.Tag, "tag", "", "Version tag for the release (e.g. v1.0.0)") + + return cmd +} + +func publishRun(opts *publishOptions) error { + dir := opts.Dir + if dir == "" { + var err error + dir, err = os.Getwd() + if err != nil { + return fmt.Errorf("could not determine working directory: %w", err) + } + } + + dir, err := filepath.Abs(dir) + if err != nil { + return fmt.Errorf("could not resolve path: %w", err) + } + + cs := opts.IO.ColorScheme() + canPrompt := opts.IO.CanPrompt() + + // Use injected client or create one from the factory HttpClient + client := opts.client + host := opts.host + + var diagnostics []publishDiagnostic + + // Check for skills directory + skillsDir := filepath.Join(dir, "skills") + info, err := os.Stat(skillsDir) + if err != nil || !info.IsDir() { + return fmt.Errorf("no skills/ directory found in %s; run this command from a repository root containing a skills/ directory", dir) + } + + // Discover skill directories + entries, err := os.ReadDir(skillsDir) + if err != nil { + return fmt.Errorf("could not read skills/ directory: %w", err) + } + + var skillDirs []string + for _, e := range entries { + if e.IsDir() { + skillDirs = append(skillDirs, e.Name()) + } + } + + if len(skillDirs) == 0 { + return fmt.Errorf("no skill directories found in %s/skills/", dir) + } + + for _, dirName := range skillDirs { + skillPath := filepath.Join(skillsDir, dirName, "SKILL.md") + content, err := os.ReadFile(skillPath) + if err != nil { + diagnostics = append(diagnostics, publishDiagnostic{ + skill: dirName, + severity: "error", + message: "missing SKILL.md file", + }) + continue + } + + result, err := frontmatter.Parse(string(content)) + if err != nil { + diagnostics = append(diagnostics, publishDiagnostic{ + skill: dirName, + severity: "error", + message: fmt.Sprintf("invalid frontmatter YAML: %s", err), + }) + continue + } + + // Validate name field exists + if result.Metadata.Name == "" { + diagnostics = append(diagnostics, publishDiagnostic{ + skill: dirName, + severity: "error", + message: "missing required field: name", + }) + } else { + // Validate name matches directory + if result.Metadata.Name != dirName { + diagnostics = append(diagnostics, publishDiagnostic{ + skill: dirName, + severity: "error", + message: fmt.Sprintf("name %q does not match directory name %q", result.Metadata.Name, dirName), + }) + } + + // Validate name is spec-compliant + if !discovery.IsSpecCompliant(result.Metadata.Name) { + diagnostics = append(diagnostics, publishDiagnostic{ + skill: dirName, + severity: "error", + message: fmt.Sprintf("name %q does not follow agentskills.io naming convention (lowercase alphanumeric + hyphens)", result.Metadata.Name), + }) + } + } + + // Validate description field exists + if result.Metadata.Description == "" { + diagnostics = append(diagnostics, publishDiagnostic{ + skill: dirName, + severity: "error", + message: "missing required field: description", + }) + } else if len(result.Metadata.Description) > 1024 { + diagnostics = append(diagnostics, publishDiagnostic{ + skill: dirName, + severity: "warning", + message: fmt.Sprintf("description is %d chars (recommended max: 1024)", len(result.Metadata.Description)), + }) + } + + // Validate allowed-tools is string, not array + if raw, ok := result.RawYAML["allowed-tools"]; ok { + if _, isSlice := raw.([]interface{}); isSlice { + diagnostics = append(diagnostics, publishDiagnostic{ + skill: dirName, + severity: "error", + message: "allowed-tools must be a string (space-delimited), not an array", + }) + } + } + + // Check for install metadata that should be stripped + if meta, ok := result.RawYAML["metadata"].(map[string]interface{}); ok { + githubKeys := findGitHubMetadataKeys(meta) + if len(githubKeys) > 0 { + if opts.Fix { + fixed, fixErr := stripGitHubMetadata(string(content)) + if fixErr != nil { + diagnostics = append(diagnostics, publishDiagnostic{ + skill: dirName, + severity: "error", + message: fmt.Sprintf("could not strip install metadata: %s", fixErr), + }) + } else if writeErr := os.WriteFile(skillPath, []byte(fixed), 0o644); writeErr != nil { + diagnostics = append(diagnostics, publishDiagnostic{ + skill: dirName, + severity: "error", + message: fmt.Sprintf("could not write fixed SKILL.md: %s", writeErr), + }) + } else { + diagnostics = append(diagnostics, publishDiagnostic{ + skill: dirName, + severity: "fixed", + message: fmt.Sprintf("stripped install metadata: %s", strings.Join(githubKeys, ", ")), + }) + } + } else { + diagnostics = append(diagnostics, publishDiagnostic{ + skill: dirName, + severity: "error", + message: fmt.Sprintf("contains install metadata that must be stripped: %s (use --fix)", strings.Join(githubKeys, ", ")), + }) + } + } + } + + // Recommended: license field + if result.Metadata.License == "" { + if _, ok := result.RawYAML["license"]; !ok { + diagnostics = append(diagnostics, publishDiagnostic{ + skill: dirName, + severity: "warning", + message: "recommended field missing: license", + }) + } + } + + // Recommended: body length + bodyLines := strings.Count(result.Body, "\n") + 1 + if bodyLines > 500 { + diagnostics = append(diagnostics, publishDiagnostic{ + skill: dirName, + severity: "warning", + message: fmt.Sprintf("skill body is %d lines (recommended max: 500 for efficient context)", bodyLines), + }) + } + } + + // Check for installed skill directories that should be gitignored + installedDirDiags := checkInstalledSkillDirs(opts.GitClient, dir) + diagnostics = append(diagnostics, installedDirDiags...) + + // Remote repository checks (best-effort) + owner, repo := detectGitHubRemote(opts.GitClient) + hasTopic := false + var existingTags []tagEntry + if owner != "" && repo != "" { + // Create API client for remote checks if not already injected + if client == nil { + httpClient, httpErr := opts.HttpClient() + if httpErr == nil { + apiClient := api.NewClientFromHTTP(httpClient) + cfg, cfgErr := opts.Config() + if cfgErr == nil { + host, _ = cfg.Authentication().DefaultHost() + client = apiClient + } + } + } + + if client != nil { + // Security and ruleset checks (advisory, always shown) + securityDiags := checkSecuritySettings(client, host, owner, repo, skillsDir) + diagnostics = append(diagnostics, securityDiags...) + + rulesetDiags := checkTagProtection(client, host, owner, repo) + diagnostics = append(diagnostics, rulesetDiags...) + + // Check topic (needed for publish flow, not a blocking error) + hasTopic = repoHasTopic(client, host, owner, repo) + + // Fetch existing tags (needed for version suggestion) + existingTags = fetchTags(client, host, owner, repo) + } + } else { + diagnostics = append(diagnostics, detectMissingRepoDiagnostic(opts.GitClient, dir)...) + } + + // Render diagnostics + errors, warnings, fixes := 0, 0, 0 + for _, d := range diagnostics { + switch d.severity { + case "error": + errors++ + case "warning": + warnings++ + case "fixed": + fixes++ + } + } + + if canPrompt { + renderDiagnosticsTTY(opts, skillDirs, diagnostics, errors, warnings, fixes, owner, repo) + } else { + renderDiagnosticsPlain(opts, diagnostics, errors, warnings) + } + + // Generate Claude Code plugin manifest if requested + if opts.Plugins { + pluginDiags := generateClaudePlugin(dir, skillDirs, owner, repo) + for _, d := range pluginDiags { + switch d.severity { + case "error": + fmt.Fprintf(opts.IO.ErrOut, "%s %s\n", cs.FailureIcon(), d.message) + default: + fmt.Fprintf(opts.IO.Out, "%s %s\n", cs.SuccessIcon(), d.message) + } + } + } + + if errors > 0 { + return fmt.Errorf("validation failed with %d error(s)", errors) + } + + // --- Publish flow --- + if opts.DryRun { + fmt.Fprintf(opts.IO.ErrOut, "\nDry run complete. Use without --dry-run to publish.\n") + return nil + } + + if owner == "" || repo == "" { + fmt.Fprintf(opts.IO.ErrOut, "\nValidation passed. Set up a GitHub remote to publish.\n") + return nil + } + + if !canPrompt && opts.Tag == "" { + fmt.Fprintf(opts.IO.ErrOut, "\nValidation passed. Use --tag to publish non-interactively.\n") + return nil + } + + if client == nil { + fmt.Fprintf(opts.IO.ErrOut, "\nValidation passed but could not create API client. Check your authentication configuration.\n") + return nil + } + + fmt.Fprintf(opts.IO.ErrOut, "\nPublishing to %s/%s...\n\n", owner, repo) + + return runPublishRelease(opts, client, host, owner, repo, dir, hasTopic, existingTags) +} + +// repoHasTopic checks whether the repo has the agent-skills topic. +func repoHasTopic(client *api.Client, host, owner, repo string) bool { + if client == nil { + return false + } + apiPath := fmt.Sprintf("repos/%s/%s/topics", owner, repo) + var resp repoTopicsResponse + if err := client.REST(host, "GET", apiPath, nil, &resp); err != nil { + return false + } + for _, t := range resp.Names { + if t == "agent-skills" { + return true + } + } + return false +} + +// fetchTags returns the most recent tags from the repo. +func fetchTags(client *api.Client, host, owner, repo string) []tagEntry { + if client == nil { + return nil + } + apiPath := fmt.Sprintf("repos/%s/%s/tags?per_page=10", owner, repo) + var tags []tagEntry + if err := client.REST(host, "GET", apiPath, nil, &tags); err != nil { + return nil + } + return tags +} + +// runPublishRelease handles the interactive publish flow: topic, tag, release, immutability. +func runPublishRelease(opts *publishOptions, client *api.Client, host, owner, repo, dir string, hasTopic bool, existingTags []tagEntry) error { + cs := opts.IO.ColorScheme() + canPrompt := opts.IO.CanPrompt() + + // 1. Add topic if missing + if !hasTopic { + addTopic := true + if canPrompt { + var err error + addTopic, err = opts.Prompter.Confirm( + fmt.Sprintf("Add \"agent-skills\" topic to %s/%s? (required for discoverability)", owner, repo), true) + if err != nil { + return err + } + } + if addTopic { + if err := addAgentSkillsTopic(client, host, owner, repo); err != nil { + fmt.Fprintf(opts.IO.ErrOut, "%s Could not add topic: %v\n", cs.WarningIcon(), err) + fmt.Fprintf(opts.IO.ErrOut, " Add it manually: gh repo edit %s/%s --add-topic agent-skills\n", owner, repo) + } else { + fmt.Fprintf(opts.IO.Out, "%s Added \"agent-skills\" topic\n", cs.SuccessIcon()) + } + } + } + + // 2. Determine tag + tag := opts.Tag + if tag == "" { + suggested := "v1.0.0" + if len(existingTags) > 0 { + if next := suggestNextTag(existingTags[0].Name); next != "" { + suggested = next + } + } + + if canPrompt { + strategies := []string{ + fmt.Sprintf("Semver (recommended) — %s", suggested), + "Custom tag", + } + idx, err := opts.Prompter.Select("Tagging strategy:", "", strategies) + if err != nil { + return err + } + + if idx == 0 { + tag = suggested + edited, err := opts.Prompter.Input(fmt.Sprintf("Version tag [%s]:", suggested), suggested) + if err != nil { + return err + } + if edited != "" { + tag = edited + } + } else { + custom, err := opts.Prompter.Input("Tag:", "") + if err != nil { + return err + } + if custom == "" { + return fmt.Errorf("tag is required") + } + tag = custom + } + } else { + return fmt.Errorf("--tag is required for non-interactive publish") + } + } + + // Validate tag doesn't already exist + for _, t := range existingTags { + if t.Name == tag { + return fmt.Errorf("tag %s already exists — choose a different version", tag) + } + } + + // 3. Offer to enable immutable releases + immutableEnabled := checkImmutableReleases(client, host, owner, repo) + if !immutableEnabled && canPrompt { + enableImmutable, err := opts.Prompter.Confirm( + "Enable immutable releases? (prevents tampering with published releases)", true) + if err != nil { + return err + } + if enableImmutable { + if err := enableImmutableReleases(client, host, owner, repo); err != nil { + fmt.Fprintf(opts.IO.ErrOut, "%s Could not enable immutable releases: %v\n", cs.WarningIcon(), err) + fmt.Fprintf(opts.IO.ErrOut, " Enable manually in Settings → General → Releases\n") + } else { + fmt.Fprintf(opts.IO.Out, "%s Enabled immutable releases\n", cs.SuccessIcon()) + } + } + } + + // 4. Inform if not on default branch + var currentBranch string + if opts.GitClient != nil { + bc := *opts.GitClient + bc.RepoDir = dir + if b, err := bc.CurrentBranch(context.Background()); err == nil { + currentBranch = b + } + } + defaultBranch := detectDefaultBranch(client, host, owner, repo) + if currentBranch != "" && defaultBranch != "" && currentBranch != defaultBranch { + fmt.Fprintf(opts.IO.ErrOut, "%s Publishing from branch %q (default is %q)\n", cs.WarningIcon(), currentBranch, defaultBranch) + } + + // 5. Confirm and create release + if canPrompt { + confirmed, err := opts.Prompter.Confirm( + fmt.Sprintf("Create release %s with auto-generated notes?", tag), true) + if err != nil { + return err + } + if !confirmed { + fmt.Fprintf(opts.IO.ErrOut, "Publish cancelled.\n") + return nil + } + } + + // Create release via REST API + releaseBody := map[string]interface{}{ + "tag_name": tag, + "generate_release_notes": true, + } + if currentBranch != "" { + releaseBody["target_commitish"] = currentBranch + } + releaseJSON, err := json.Marshal(releaseBody) + if err != nil { + return fmt.Errorf("failed to serialize release request: %w", err) + } + + releasePath := fmt.Sprintf("repos/%s/%s/releases", owner, repo) + var releaseResp struct { + HTMLURL string `json:"html_url"` + } + if err := client.REST(host, "POST", releasePath, bytes.NewReader(releaseJSON), &releaseResp); err != nil { + return fmt.Errorf("failed to create release: %w", err) + } + + fmt.Fprintf(opts.IO.Out, "%s Published %s\n", cs.SuccessIcon(), tag) + fmt.Fprintf(opts.IO.Out, "%s Install with: gh skills install %s/%s\n", cs.SuccessIcon(), owner, repo) + fmt.Fprintf(opts.IO.Out, "%s Pin with: gh skills install %s/%s --pin %s\n", cs.SuccessIcon(), owner, repo, tag) + + return nil +} + +// detectDefaultBranch returns the default branch of the remote repo via the API. +func detectDefaultBranch(client *api.Client, host, owner, repo string) string { + if client == nil { + return "" + } + var result struct { + DefaultBranch string `json:"default_branch"` + } + if err := client.REST(host, "GET", fmt.Sprintf("repos/%s/%s", owner, repo), nil, &result); err != nil { + return "" + } + return result.DefaultBranch +} + +// addAgentSkillsTopic adds the "agent-skills" topic to the repo, preserving existing topics. +func addAgentSkillsTopic(client *api.Client, host, owner, repo string) error { + apiPath := fmt.Sprintf("repos/%s/%s/topics", owner, repo) + + // Fetch existing topics + var resp repoTopicsResponse + if err := client.REST(host, "GET", apiPath, nil, &resp); err != nil { + return fmt.Errorf("could not fetch existing topics: %w", err) + } + + // Deduplicate: only add if not already present + for _, t := range resp.Names { + if t == "agent-skills" { + return nil + } + } + + topics := append(resp.Names, "agent-skills") + topicsJSON, err := json.Marshal(map[string][]string{"names": topics}) + if err != nil { + return fmt.Errorf("could not serialize topics: %w", err) + } + return client.REST(host, "PUT", apiPath, bytes.NewReader(topicsJSON), nil) +} + +// checkImmutableReleases checks if immutable releases are enabled for the repo. +func checkImmutableReleases(client *api.Client, host, owner, repo string) bool { + if client == nil { + return false + } + apiPath := fmt.Sprintf("repos/%s/%s/immutable-releases", owner, repo) + var resp struct { + Enabled bool `json:"enabled"` + } + if err := client.REST(host, "GET", apiPath, nil, &resp); err != nil { + return false + } + return resp.Enabled +} + +// enableImmutableReleases enables immutable releases for the repo. +func enableImmutableReleases(client *api.Client, host, owner, repo string) error { + apiPath := fmt.Sprintf("repos/%s/%s/immutable-releases", owner, repo) + body := bytes.NewReader([]byte(`{"enabled":true}`)) + return client.REST(host, "PATCH", apiPath, body, nil) +} + +// checkTagProtection checks whether tag protection rulesets are enabled. +func checkTagProtection(client *api.Client, host, owner, repo string) []publishDiagnostic { + if client == nil { + return nil + } + apiPath := fmt.Sprintf("repos/%s/%s/rulesets", owner, repo) + var rulesets []rulesetsResponse + if err := client.REST(host, "GET", apiPath, nil, &rulesets); err != nil { + return nil + } + + for _, rs := range rulesets { + if rs.Target == "tag" && rs.Enforcement == "active" { + return nil + } + } + + return []publishDiagnostic{{ + severity: "warning", + message: "no active tag protection rulesets found — consider protecting tags to ensure immutable releases (Settings → Rules → Rulesets)", + }} +} + +// checkSecuritySettings checks whether recommended security features are enabled. +func checkSecuritySettings(client *api.Client, host, owner, repo, skillsDir string) []publishDiagnostic { + if client == nil { + return nil + } + apiPath := fmt.Sprintf("repos/%s/%s", owner, repo) + var resp repoSecurityResponse + if err := client.REST(host, "GET", apiPath, nil, &resp); err != nil { + return nil + } + + if resp.SecurityAndAnalysis == nil { + return nil + } + + var diagnostics []publishDiagnostic + sa := resp.SecurityAndAnalysis + + if sa.SecretScanning == nil || sa.SecretScanning.Status != "enabled" { + diagnostics = append(diagnostics, publishDiagnostic{ + severity: "warning", + message: "secret scanning is not enabled — recommended to prevent accidental credential exposure (gh repo edit --enable-secret-scanning)", + }) + } + + if sa.SecretScanningPushProtection == nil || sa.SecretScanningPushProtection.Status != "enabled" { + diagnostics = append(diagnostics, publishDiagnostic{ + severity: "warning", + message: "secret scanning push protection is not enabled — blocks pushes containing secrets (gh repo edit --enable-secret-scanning-push-protection)", + }) + } + + hasCode, hasManifests := detectCodeAndManifests(skillsDir) + + if hasCode { + alertsPath := fmt.Sprintf("repos/%s/%s/code-scanning/alerts?per_page=1&state=open", owner, repo) + if err := client.REST(host, "GET", alertsPath, nil, new([]interface{})); err != nil { + diagnostics = append(diagnostics, publishDiagnostic{ + severity: "info", + message: "skills include code files but code scanning does not appear to be configured (Settings → Code security → Code scanning)", + }) + } + } + + if hasManifests { + dependabotPath := fmt.Sprintf("repos/%s/%s/vulnerability-alerts", owner, repo) + if err := client.REST(host, "GET", dependabotPath, nil, nil); err != nil { + diagnostics = append(diagnostics, publishDiagnostic{ + severity: "info", + message: "skills include dependency manifests but Dependabot alerts do not appear to be enabled (Settings → Code security → Dependabot)", + }) + } + } + + return diagnostics +} + +// codeExtensions are file extensions that indicate code is present. +var codeExtensions = map[string]bool{ + ".go": true, ".py": true, ".js": true, ".ts": true, ".rb": true, + ".rs": true, ".java": true, ".cs": true, ".sh": true, ".bash": true, + ".zsh": true, ".ps1": true, ".swift": true, ".kt": true, ".c": true, + ".cpp": true, ".h": true, ".php": true, ".pl": true, ".lua": true, +} + +// manifestFiles are dependency manifest filenames. +var manifestFiles = map[string]bool{ + "package.json": true, "package-lock.json": true, "yarn.lock": true, + "go.mod": true, "go.sum": true, "Cargo.toml": true, "Cargo.lock": true, + "requirements.txt": true, "Pipfile": true, "Pipfile.lock": true, + "pyproject.toml": true, "poetry.lock": true, "Gemfile": true, + "Gemfile.lock": true, "pom.xml": true, "build.gradle": true, + "composer.json": true, "composer.lock": true, +} + +// detectCodeAndManifests walks the skills directory looking for code files +// and dependency manifests. +func detectCodeAndManifests(skillsDir string) (hasCode, hasManifests bool) { + _ = filepath.Walk(skillsDir, func(path string, info os.FileInfo, err error) error { + if err != nil { + return err + } + if info.IsDir() { + return nil + } + ext := filepath.Ext(info.Name()) + if codeExtensions[ext] { + hasCode = true + } + if manifestFiles[info.Name()] { + hasManifests = true + } + if hasCode && hasManifests { + return filepath.SkipAll + } + return nil + }) + return +} + +// checkInstalledSkillDirs warns when agent host skill directories exist +// in the repo and are not gitignored. +func checkInstalledSkillDirs(gitClient *git.Client, repoDir string) []publishDiagnostic { + var diagnostics []publishDiagnostic + + for _, relPath := range registry.UniqueProjectDirs() { + absPath := filepath.Join(repoDir, relPath) + if _, err := os.Stat(absPath); os.IsNotExist(err) { + continue + } + + if gitClient != nil { + ic := *gitClient + ic.RepoDir = repoDir + if ic.IsIgnored(context.Background(), relPath) { + continue + } + } + + diagnostics = append(diagnostics, publishDiagnostic{ + severity: "warning", + message: fmt.Sprintf( + "%s/ contains installed skills and should be added to .gitignore to avoid publishing other authors' content", + relPath), + }) + } + + return diagnostics +} + +// semverPattern matches v-prefixed semver tags (e.g. v1.2.3). +var semverPattern = regexp.MustCompile(`^v?(\d+)\.(\d+)\.(\d+)$`) + +// suggestNextTag increments the patch version of a semver tag. +func suggestNextTag(latest string) string { + m := semverPattern.FindStringSubmatch(latest) + if m == nil { + return "" + } + + prefix := "" + if strings.HasPrefix(latest, "v") { + prefix = "v" + } + + major, minor := m[1], m[2] + patch := 0 + fmt.Sscanf(m[3], "%d", &patch) + + return fmt.Sprintf("%s%s.%s.%d", prefix, major, minor, patch+1) +} + +// detectGitHubRemote attempts to detect the GitHub owner/repo from git remotes. +func detectGitHubRemote(gitClient *git.Client) (owner, repo string) { + if gitClient == nil { + return "", "" + } + + // Try origin first + if url, err := gitClient.RemoteURL(context.Background(), "origin"); err == nil { + if o, r := parseGitHubURL(url); o != "" { + return o, r + } + } + + // Fall back to any remote that points to GitHub + remotes, err := gitClient.Remotes(context.Background()) + if err != nil { + return "", "" + } + 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 + } + } + } + return "", "" +} + +// parseGitHubURL extracts owner/repo from a GitHub remote URL. +// Only GitHub.com URLs are recognized. +func parseGitHubURL(rawURL string) (owner, repo string) { + u, err := giturl.ParseURL(rawURL) + if err != nil { + return "", "" + } + r, err := ghrepo.FromURL(u) + if err != nil { + return "", "" + } + // Only match github.com — the default GitHub host. + host := strings.ToLower(r.RepoHost()) + if host != ghinstance.Default() { + return "", "" + } + return r.RepoOwner(), r.RepoName() +} + +// detectMissingRepoDiagnostic explains why remote checks were skipped. +func detectMissingRepoDiagnostic(gitClient *git.Client, dir string) []publishDiagnostic { + if gitClient == nil { + return nil + } + + dc := *gitClient + dc.RepoDir = dir + if _, err := dc.GitDir(context.Background()); err != nil { + return []publishDiagnostic{{ + severity: "warning", + message: "not a git repository — initialize with: git init && gh repo create", + }} + } + + remotes, err := dc.Remotes(context.Background()) + if err != nil || len(remotes) == 0 { + return []publishDiagnostic{{ + severity: "warning", + message: "no git remote found — create a GitHub repository with: gh repo create", + }} + } + + var urls []string + for _, r := range remotes { + if url, err := dc.RemoteURL(context.Background(), r.Name); err == nil { + urls = append(urls, url) + } + } + return []publishDiagnostic{{ + severity: "warning", + message: fmt.Sprintf("remote %q is not a GitHub repository — skills must be hosted on GitHub for discovery", strings.Join(urls, ", ")), + }} +} + +func renderDiagnosticsTTY(opts *publishOptions, skillDirs []string, diagnostics []publishDiagnostic, errors, warnings, fixes int, owner, repo string) { + cs := opts.IO.ColorScheme() + + // Separate info messages from errors/warnings for cleaner output + var infos, issues []publishDiagnostic + for _, d := range diagnostics { + if d.severity == "info" { + infos = append(infos, d) + } else { + issues = append(issues, d) + } + } + + if len(issues) == 0 && fixes == 0 { + fmt.Fprintf(opts.IO.Out, "%s %d skill(s) validated successfully\n", cs.SuccessIcon(), len(skillDirs)) + } else { + for _, d := range issues { + var prefix string + switch d.severity { + case "error": + prefix = cs.FailureIcon() + case "warning": + prefix = cs.WarningIcon() + case "fixed": + prefix = cs.SuccessIcon() + default: + prefix = cs.FailureIcon() + } + if d.skill != "" { + fmt.Fprintf(opts.IO.Out, "%s %s: %s\n", prefix, cs.Bold(d.skill), d.message) + } else { + fmt.Fprintf(opts.IO.Out, "%s %s\n", prefix, d.message) + } + } + + fmt.Fprintln(opts.IO.Out) + if fixes > 0 { + fmt.Fprintf(opts.IO.Out, "Fixed %d issue(s)\n", fixes) + } + if errors > 0 { + fmt.Fprintf(opts.IO.Out, "%s, %s\n", + cs.Red(fmt.Sprintf("%d error(s)", errors)), + cs.Yellow(fmt.Sprintf("%d warning(s)", warnings))) + } else { + fmt.Fprintf(opts.IO.Out, "%s\n", cs.Yellow(fmt.Sprintf("%d warning(s)", warnings))) + } + } + + // Always show info messages + for _, d := range infos { + fmt.Fprintf(opts.IO.ErrOut, "\n%s\n", d.message) + } + + if errors == 0 { + if owner != "" && repo != "" { + fmt.Fprintf(opts.IO.ErrOut, "\n%s Repository: %s/%s\n", cs.Green("Ready to publish!"), owner, repo) + } else { + fmt.Fprintf(opts.IO.ErrOut, "\n%s Ensure the repository has the \"agent-skills\" topic.\n", cs.Green("Ready to publish!")) + } + } +} + +func renderDiagnosticsPlain(opts *publishOptions, diagnostics []publishDiagnostic, errors, warnings int) { + for _, d := range diagnostics { + if d.severity == "info" { + continue + } + fmt.Fprintf(opts.IO.Out, "%s\t%s\t%s\n", d.severity, d.skill, d.message) + } + if errors == 0 && warnings == 0 { + fmt.Fprintf(opts.IO.Out, "ok\n") + } +} + +// findGitHubMetadataKeys returns metadata keys with the "github-" prefix. +func findGitHubMetadataKeys(meta map[string]interface{}) []string { + var keys []string + for k := range meta { + if strings.HasPrefix(k, "github-") { + keys = append(keys, k) + } + } + sort.Strings(keys) + return keys +} + +// stripGitHubMetadata removes github-* keys from the metadata map and re-serializes. +func stripGitHubMetadata(content string) (string, error) { + result, err := frontmatter.Parse(content) + if err != nil { + return "", err + } + + meta, ok := result.RawYAML["metadata"].(map[string]interface{}) + if !ok { + return content, nil + } + + for k := range meta { + if strings.HasPrefix(k, "github-") { + delete(meta, k) + } + } + + if len(meta) == 0 { + delete(result.RawYAML, "metadata") + } else { + result.RawYAML["metadata"] = meta + } + + return frontmatter.Serialize(result.RawYAML, result.Body) +} + +// claudePluginJSON is the .claude-plugin/plugin.json structure. +type claudePluginJSON struct { + Name string `json:"name"` + Description string `json:"description,omitempty"` + Version string `json:"version,omitempty"` + Author *claudeAuthor `json:"author,omitempty"` + Homepage string `json:"homepage,omitempty"` + Repository string `json:"repository,omitempty"` + License string `json:"license,omitempty"` + Keywords []string `json:"keywords,omitempty"` +} + +type claudeAuthor struct { + Name string `json:"name"` +} + +// claudeMarketplaceJSON is the .claude-plugin/marketplace.json structure. +type claudeMarketplaceJSON struct { + Name string `json:"name"` + Owner claudeAuthor `json:"owner"` + Plugins []claudeMarketplacePlugin `json:"plugins"` +} + +type claudeMarketplacePlugin struct { + Name string `json:"name"` + Source string `json:"source"` + Description string `json:"description,omitempty"` +} + +// generateClaudePlugin creates .claude-plugin/plugin.json (and optionally +// marketplace.json for multi-skill repos). +func generateClaudePlugin(dir string, skillDirs []string, owner, repo string) []publishDiagnostic { + var diags []publishDiagnostic + + pluginDir := filepath.Join(dir, ".claude-plugin") + pluginPath := filepath.Join(pluginDir, "plugin.json") + + // Don't overwrite existing plugin.json + if _, err := os.Stat(pluginPath); err == nil { + diags = append(diags, publishDiagnostic{ + severity: "info", + message: ".claude-plugin/plugin.json already exists (skipped)", + }) + return diags + } + + pluginName := filepath.Base(dir) + if repo != "" { + pluginName = repo + } + + description := buildPluginDescription(dir, skillDirs) + + plugin := claudePluginJSON{ + Name: pluginName, + Description: description, + Version: "1.0.0", + Keywords: []string{"agent-skills"}, + } + + if owner != "" && repo != "" { + plugin.Repository = fmt.Sprintf("https://github.com/%s/%s", owner, repo) + plugin.Homepage = fmt.Sprintf("https://github.com/%s/%s", owner, repo) + plugin.Author = &claudeAuthor{Name: owner} + } + + // Collect license from any skill + for _, skillName := range skillDirs { + skillPath := filepath.Join(dir, "skills", skillName, "SKILL.md") + content, err := os.ReadFile(skillPath) + if err != nil { + continue + } + result, err := frontmatter.Parse(string(content)) + if err != nil { + continue + } + if result.Metadata.License != "" { + plugin.License = result.Metadata.License + break + } + } + + if err := os.MkdirAll(pluginDir, 0o755); err != nil { + diags = append(diags, publishDiagnostic{ + severity: "error", + message: fmt.Sprintf("could not create .claude-plugin/: %v", err), + }) + return diags + } + + data, err := json.MarshalIndent(plugin, "", " ") + if err != nil { + diags = append(diags, publishDiagnostic{ + severity: "error", + message: fmt.Sprintf("could not serialize plugin.json: %v", err), + }) + return diags + } + + if err := os.WriteFile(pluginPath, append(data, '\n'), 0o644); err != nil { + diags = append(diags, publishDiagnostic{ + severity: "error", + message: fmt.Sprintf("could not write plugin.json: %v", err), + }) + return diags + } + + diags = append(diags, publishDiagnostic{ + severity: "info", + message: fmt.Sprintf("generated .claude-plugin/plugin.json for %q with %d skill(s)", pluginName, len(skillDirs)), + }) + + // Generate marketplace.json for multi-skill repos with a GitHub remote + if len(skillDirs) > 1 && owner != "" && repo != "" { + marketplacePath := filepath.Join(pluginDir, "marketplace.json") + if _, err := os.Stat(marketplacePath); err != nil { + mDiags := generateMarketplace(marketplacePath, pluginName, owner, skillDirs, dir) + diags = append(diags, mDiags...) + } + } + + return diags +} + +// generateMarketplace creates a marketplace.json for plugin marketplace discovery. +func generateMarketplace(path, pluginName, owner string, skillDirs []string, dir string) []publishDiagnostic { + desc := buildPluginDescription(dir, skillDirs) + plugins := []claudeMarketplacePlugin{{ + Name: pluginName, + Source: ".", + Description: desc, + }} + + marketplace := claudeMarketplaceJSON{ + Name: pluginName, + Owner: claudeAuthor{Name: owner}, + Plugins: plugins, + } + + data, err := json.MarshalIndent(marketplace, "", " ") + if err != nil { + return []publishDiagnostic{{ + severity: "error", + message: fmt.Sprintf("could not serialize marketplace.json: %v", err), + }} + } + + if err := os.WriteFile(path, append(data, '\n'), 0o644); err != nil { + return []publishDiagnostic{{ + severity: "error", + message: fmt.Sprintf("could not write marketplace.json: %v", err), + }} + } + + return []publishDiagnostic{{ + severity: "info", + message: "generated .claude-plugin/marketplace.json for plugin marketplace discovery", + }} +} + +// buildPluginDescription creates a description from skill names and descriptions. +func buildPluginDescription(dir string, skillDirs []string) string { + if len(skillDirs) == 1 { + skillPath := filepath.Join(dir, "skills", skillDirs[0], "SKILL.md") + if content, err := os.ReadFile(skillPath); err == nil { + if result, err := frontmatter.Parse(string(content)); err == nil && result.Metadata.Description != "" { + return result.Metadata.Description + } + } + } + + var names []string + for _, name := range skillDirs { + names = append(names, name) + } + if len(names) <= 5 { + return fmt.Sprintf("Agent skills: %s", strings.Join(names, ", ")) + } + return fmt.Sprintf("Agent skills collection with %d skills", len(names)) +} diff --git a/pkg/cmd/skills/publish/publish_test.go b/pkg/cmd/skills/publish/publish_test.go new file mode 100644 index 000000000..56e3b1e0a --- /dev/null +++ b/pkg/cmd/skills/publish/publish_test.go @@ -0,0 +1,1059 @@ +package publish + +import ( + "bytes" + "encoding/json" + "fmt" + "net/http" + "os" + "os/exec" + "path/filepath" + "strings" + "testing" + + "github.com/cli/cli/v2/api" + "github.com/cli/cli/v2/git" + "github.com/cli/cli/v2/pkg/cmdutil" + "github.com/cli/cli/v2/pkg/httpmock" + "github.com/cli/cli/v2/pkg/iostreams" + "github.com/stretchr/testify/require" +) + +func testPublishGitClient(t *testing.T, remoteURLs map[string]string) *git.Client { + t.Helper() + dir := t.TempDir() + runGit := func(args ...string) { + t.Helper() + cmd := exec.Command("git", append([]string{"-C", dir}, args...)...) + cmd.Env = append(os.Environ(), "GIT_CONFIG_NOSYSTEM=1", "HOME="+dir) + out, err := cmd.CombinedOutput() + require.NoError(t, err, "git %v: %s", args, out) + } + runGit("init", "--initial-branch=main") + runGit("config", "user.email", "monalisa@github.com") + runGit("config", "user.name", "Monalisa Octocat") + for name, url := range remoteURLs { + runGit("remote", "add", name, url) + } + return &git.Client{RepoDir: dir} +} + +func TestPublishCmd_Help(t *testing.T) { + ios, _, _, _ := iostreams.Test() + f := stubFactory(ios) + cmd := NewCmdPublish(&f, nil) + if cmd.Use == "" { + t.Error("publish command has no Use string") + } + if cmd.Short == "" { + t.Error("publish command has no Short description") + } +} + +func TestPublishCmd_Alias(t *testing.T) { + ios, _, _, _ := iostreams.Test() + f := stubFactory(ios) + cmd := NewCmdPublish(&f, nil) + found := false + for _, alias := range cmd.Aliases { + if alias == "validate" { + found = true + break + } + } + if !found { + t.Error("publish command should have 'validate' alias") + } +} + +func TestPublish_ValidSkill(t *testing.T) { + dir := t.TempDir() + skillDir := filepath.Join(dir, "skills", "git-commit") + if err := os.MkdirAll(skillDir, 0o755); err != nil { + t.Fatal(err) + } + + content := `--- +name: git-commit +description: A skill for writing good git commits +allowed-tools: git +license: MIT +--- +You are a git commit expert. +` + if err := os.WriteFile(filepath.Join(skillDir, "SKILL.md"), []byte(content), 0o644); err != nil { + t.Fatal(err) + } + + ios, _, stdout, _ := iostreams.Test() + + reg := &httpmock.Registry{} + defer reg.Verify(t) + reg.Register( + httpmock.REST("GET", "repos/test/skills-repo/topics"), + httpmock.JSONResponse(map[string]interface{}{ + "names": []string{"agent-skills"}, + }), + ) + reg.Register( + httpmock.REST("GET", "repos/test/skills-repo/tags"), + httpmock.JSONResponse([]map[string]interface{}{ + {"name": "v1.0.0"}, + }), + ) + reg.Register( + httpmock.REST("GET", "repos/test/skills-repo/rulesets"), + httpmock.JSONResponse([]map[string]interface{}{ + {"id": 1, "name": "tags", "target": "tag", "enforcement": "active"}, + }), + ) + reg.Register( + httpmock.REST("GET", "repos/test/skills-repo"), + httpmock.JSONResponse(map[string]interface{}{ + "security_and_analysis": map[string]interface{}{ + "secret_scanning": map[string]interface{}{"status": "enabled"}, + "secret_scanning_push_protection": map[string]interface{}{"status": "enabled"}, + }, + }), + ) + + opts := &publishOptions{ + IO: ios, + Dir: dir, + GitClient: testPublishGitClient(t, map[string]string{ + "origin": "https://github.com/test/skills-repo.git", + }), + client: api.NewClientFromHTTP(&http.Client{Transport: reg}), + host: "github.com", + } + + err := publishRun(opts) + if err != nil { + t.Fatalf("expected no error, got: %v", err) + } + + out := stdout.String() + if !strings.Contains(out, "ok") { + t.Errorf("expected 'ok' output, got: %s", out) + } +} + +func TestPublish_MissingName(t *testing.T) { + dir := t.TempDir() + skillDir := filepath.Join(dir, "skills", "git-commit") + if err := os.MkdirAll(skillDir, 0o755); err != nil { + t.Fatal(err) + } + + content := `--- +description: A skill for writing good git commits +--- +Body text. +` + if err := os.WriteFile(filepath.Join(skillDir, "SKILL.md"), []byte(content), 0o644); err != nil { + t.Fatal(err) + } + + ios, _, stdout, _ := iostreams.Test() + + opts := &publishOptions{ + IO: ios, + Dir: dir, + } + + err := publishRun(opts) + if err == nil { + t.Fatal("expected error for missing name") + } + + out := stdout.String() + if !strings.Contains(out, "missing required field: name") { + t.Errorf("expected name error in output, got: %s", out) + } +} + +func TestPublish_NameMismatch(t *testing.T) { + dir := t.TempDir() + skillDir := filepath.Join(dir, "skills", "git-commit") + if err := os.MkdirAll(skillDir, 0o755); err != nil { + t.Fatal(err) + } + + content := `--- +name: wrong-name +description: A skill +--- +Body. +` + if err := os.WriteFile(filepath.Join(skillDir, "SKILL.md"), []byte(content), 0o644); err != nil { + t.Fatal(err) + } + + ios, _, stdout, _ := iostreams.Test() + + opts := &publishOptions{ + IO: ios, + Dir: dir, + } + + err := publishRun(opts) + if err == nil { + t.Fatal("expected error for name mismatch") + } + + out := stdout.String() + if !strings.Contains(out, "does not match directory name") { + t.Errorf("expected name mismatch error, got: %s", out) + } +} + +func TestPublish_NonSpecCompliantName(t *testing.T) { + dir := t.TempDir() + skillDir := filepath.Join(dir, "skills", "My_Skill") + if err := os.MkdirAll(skillDir, 0o755); err != nil { + t.Fatal(err) + } + + content := `--- +name: My_Skill +description: A skill with non-compliant name +--- +Body. +` + if err := os.WriteFile(filepath.Join(skillDir, "SKILL.md"), []byte(content), 0o644); err != nil { + t.Fatal(err) + } + + ios, _, stdout, _ := iostreams.Test() + + opts := &publishOptions{ + IO: ios, + Dir: dir, + } + + err := publishRun(opts) + if err == nil { + t.Fatal("expected error for non-spec-compliant name") + } + + out := stdout.String() + if !strings.Contains(out, "naming convention") { + t.Errorf("expected naming convention error, got: %s", out) + } +} + +func TestPublish_AllowedToolsArray(t *testing.T) { + dir := t.TempDir() + skillDir := filepath.Join(dir, "skills", "bad-tools") + if err := os.MkdirAll(skillDir, 0o755); err != nil { + t.Fatal(err) + } + + content := `--- +name: bad-tools +description: A skill with array allowed-tools +allowed-tools: + - git + - curl +--- +Body. +` + if err := os.WriteFile(filepath.Join(skillDir, "SKILL.md"), []byte(content), 0o644); err != nil { + t.Fatal(err) + } + + ios, _, stdout, _ := iostreams.Test() + + opts := &publishOptions{ + IO: ios, + Dir: dir, + } + + err := publishRun(opts) + if err == nil { + t.Fatal("expected error for array allowed-tools") + } + + out := stdout.String() + if !strings.Contains(out, "allowed-tools must be a string") { + t.Errorf("expected allowed-tools error, got: %s", out) + } +} + +func TestPublish_StripMetadata(t *testing.T) { + dir := t.TempDir() + skillDir := filepath.Join(dir, "skills", "test-skill") + if err := os.MkdirAll(skillDir, 0o755); err != nil { + t.Fatal(err) + } + + content := `--- +name: test-skill +description: A test skill +metadata: + github-owner: someone + github-repo: something + github-ref: v1.0.0 + github-sha: abc123 + github-tree-sha: def456 +--- +Body. +` + skillPath := filepath.Join(skillDir, "SKILL.md") + if err := os.WriteFile(skillPath, []byte(content), 0o644); err != nil { + t.Fatal(err) + } + + ios, _, _, _ := iostreams.Test() + + opts := &publishOptions{ + IO: ios, + Dir: dir, + Fix: true, + } + + err := publishRun(opts) + if err != nil { + t.Fatalf("expected no error with --fix, got: %v", err) + } + + fixed, err := os.ReadFile(skillPath) + if err != nil { + t.Fatal(err) + } + + fixedStr := string(fixed) + if strings.Contains(fixedStr, "github-owner") { + t.Errorf("expected github-owner to be stripped, got:\n%s", fixedStr) + } + if strings.Contains(fixedStr, "github-sha") { + t.Errorf("expected github-sha to be stripped, got:\n%s", fixedStr) + } + if strings.Contains(fixedStr, "metadata:") { + t.Errorf("expected empty metadata map to be removed, got:\n%s", fixedStr) + } +} + +func TestPublish_MetadataWithoutFix(t *testing.T) { + dir := t.TempDir() + skillDir := filepath.Join(dir, "skills", "test-skill") + if err := os.MkdirAll(skillDir, 0o755); err != nil { + t.Fatal(err) + } + + content := `--- +name: test-skill +description: A test skill +metadata: + github-owner: someone + github-sha: abc123 +--- +Body. +` + if err := os.WriteFile(filepath.Join(skillDir, "SKILL.md"), []byte(content), 0o644); err != nil { + t.Fatal(err) + } + + ios, _, stdout, _ := iostreams.Test() + + opts := &publishOptions{ + IO: ios, + Dir: dir, + Fix: false, + } + + err := publishRun(opts) + if err == nil { + t.Fatal("expected error without --fix when metadata present") + } + + out := stdout.String() + if !strings.Contains(out, "install metadata") { + t.Errorf("expected install metadata error, got: %s", out) + } + if !strings.Contains(out, "--fix") { + t.Errorf("expected --fix suggestion, got: %s", out) + } +} + +func TestPublish_NoSkillsDir(t *testing.T) { + dir := t.TempDir() + ios, _, _, _ := iostreams.Test() + + opts := &publishOptions{ + IO: ios, + Dir: dir, + } + + err := publishRun(opts) + if err == nil { + t.Fatal("expected error for missing skills/ directory") + } + if !strings.Contains(err.Error(), "no skills/ directory") { + t.Errorf("expected 'no skills/ directory' error, got: %v", err) + } +} + +func TestPublish_MissingSKILLMD(t *testing.T) { + dir := t.TempDir() + skillDir := filepath.Join(dir, "skills", "empty-skill") + if err := os.MkdirAll(skillDir, 0o755); err != nil { + t.Fatal(err) + } + + ios, _, stdout, _ := iostreams.Test() + + opts := &publishOptions{ + IO: ios, + Dir: dir, + } + + err := publishRun(opts) + if err == nil { + t.Fatal("expected error for missing SKILL.md") + } + + out := stdout.String() + if !strings.Contains(out, "missing SKILL.md") { + t.Errorf("expected missing SKILL.md error, got: %s", out) + } +} + +func TestPublish_DryRun(t *testing.T) { + dir := t.TempDir() + skillDir := filepath.Join(dir, "skills", "good-skill") + if err := os.MkdirAll(skillDir, 0o755); err != nil { + t.Fatal(err) + } + + content := `--- +name: good-skill +description: A good skill +license: MIT +--- +Body. +` + if err := os.WriteFile(filepath.Join(skillDir, "SKILL.md"), []byte(content), 0o644); err != nil { + t.Fatal(err) + } + + ios, _, _, stderr := iostreams.Test() + ios.SetStdoutTTY(true) + ios.SetStderrTTY(true) + + reg := &httpmock.Registry{} + defer reg.Verify(t) + reg.Register( + httpmock.REST("GET", "repos/test/skills-repo/topics"), + httpmock.JSONResponse(map[string]interface{}{ + "names": []string{"agent-skills"}, + }), + ) + reg.Register( + httpmock.REST("GET", "repos/test/skills-repo/tags"), + httpmock.JSONResponse([]map[string]interface{}{ + {"name": "v1.0.0"}, + }), + ) + reg.Register( + httpmock.REST("GET", "repos/test/skills-repo/rulesets"), + httpmock.JSONResponse([]map[string]interface{}{ + {"id": 1, "name": "tags", "target": "tag", "enforcement": "active"}, + }), + ) + reg.Register( + httpmock.REST("GET", "repos/test/skills-repo"), + httpmock.JSONResponse(map[string]interface{}{ + "security_and_analysis": map[string]interface{}{ + "secret_scanning": map[string]interface{}{"status": "enabled"}, + "secret_scanning_push_protection": map[string]interface{}{"status": "enabled"}, + }, + }), + ) + + opts := &publishOptions{ + IO: ios, + Dir: dir, + DryRun: true, + GitClient: testPublishGitClient(t, map[string]string{ + "origin": "https://github.com/test/skills-repo.git", + }), + client: api.NewClientFromHTTP(&http.Client{Transport: reg}), + host: "github.com", + } + + err := publishRun(opts) + if err != nil { + t.Fatalf("expected no error, got: %v", err) + } + + errOut := stderr.String() + if !strings.Contains(errOut, "Dry run complete") { + t.Errorf("stderr should confirm dry run, got: %s", errOut) + } +} + +func TestPublish_LicenseWarning(t *testing.T) { + dir := t.TempDir() + skillDir := filepath.Join(dir, "skills", "no-license") + if err := os.MkdirAll(skillDir, 0o755); err != nil { + t.Fatal(err) + } + + content := `--- +name: no-license +description: A skill without license +--- +Body. +` + if err := os.WriteFile(filepath.Join(skillDir, "SKILL.md"), []byte(content), 0o644); err != nil { + t.Fatal(err) + } + + ios, _, stdout, _ := iostreams.Test() + + opts := &publishOptions{ + IO: ios, + Dir: dir, + } + + err := publishRun(opts) + if err != nil { + t.Fatalf("expected no error (warnings only), got: %v", err) + } + + out := stdout.String() + if !strings.Contains(out, "license") { + t.Errorf("expected license warning, got: %s", out) + } +} + +func TestSuggestNextTag(t *testing.T) { + tests := []struct { + input string + want string + }{ + {"v1.0.0", "v1.0.1"}, + {"v2.3.4", "v2.3.5"}, + {"1.0.0", "1.0.1"}, + {"v0.0.9", "v0.0.10"}, + {"not-semver", ""}, + {"v1", ""}, + {"v1.0", ""}, + } + for _, tt := range tests { + t.Run(tt.input, func(t *testing.T) { + got := suggestNextTag(tt.input) + if got != tt.want { + t.Errorf("suggestNextTag(%q) = %q, want %q", tt.input, got, tt.want) + } + }) + } +} + +func TestParseGitHubURL(t *testing.T) { + tests := []struct { + url string + wantOwner string + wantRepo string + }{ + {"git@github.com:github/gh-skills.git", "github", "gh-skills"}, + {"https://github.com/github/gh-skills.git", "github", "gh-skills"}, + {"https://github.com/github/gh-skills", "github", "gh-skills"}, + {"git@github.com:owner/repo.git", "owner", "repo"}, + {"https://gitlab.com/owner/repo.git", "", ""}, + {"not-a-url", "", ""}, + } + for _, tt := range tests { + t.Run(tt.url, func(t *testing.T) { + owner, repo := parseGitHubURL(tt.url) + if owner != tt.wantOwner || repo != tt.wantRepo { + t.Errorf("parseGitHubURL(%q) = (%q, %q), want (%q, %q)", tt.url, owner, repo, tt.wantOwner, tt.wantRepo) + } + }) + } +} + +func TestRepoHasTopic(t *testing.T) { + reg := &httpmock.Registry{} + defer reg.Verify(t) + reg.Register( + httpmock.REST("GET", "repos/owner/repo/topics"), + httpmock.JSONResponse(map[string]interface{}{ + "names": []string{"golang", "agent-skills"}, + }), + ) + + if !repoHasTopic(api.NewClientFromHTTP(&http.Client{Transport: reg}), "github.com", "owner", "repo") { + t.Error("expected true when topic present") + } +} + +func TestRepoHasTopic_Missing(t *testing.T) { + reg := &httpmock.Registry{} + defer reg.Verify(t) + reg.Register( + httpmock.REST("GET", "repos/owner/repo/topics"), + httpmock.JSONResponse(map[string]interface{}{ + "names": []string{"golang"}, + }), + ) + + if repoHasTopic(api.NewClientFromHTTP(&http.Client{Transport: reg}), "github.com", "owner", "repo") { + t.Error("expected false when topic missing") + } +} + +func TestFetchTags_NoTags(t *testing.T) { + reg := &httpmock.Registry{} + defer reg.Verify(t) + reg.Register( + httpmock.REST("GET", "repos/owner/repo/tags"), + httpmock.JSONResponse([]interface{}{}), + ) + + tags := fetchTags(api.NewClientFromHTTP(&http.Client{Transport: reg}), "github.com", "owner", "repo") + if len(tags) != 0 { + t.Errorf("expected no tags, got %d", len(tags)) + } +} + +func TestFetchTags_WithTags(t *testing.T) { + reg := &httpmock.Registry{} + defer reg.Verify(t) + reg.Register( + httpmock.REST("GET", "repos/owner/repo/tags"), + httpmock.JSONResponse([]map[string]interface{}{ + {"name": "v1.2.3"}, + }), + ) + + tags := fetchTags(api.NewClientFromHTTP(&http.Client{Transport: reg}), "github.com", "owner", "repo") + if len(tags) != 1 { + t.Fatalf("expected 1 tag, got %d", len(tags)) + } + if tags[0].Name != "v1.2.3" { + t.Errorf("expected v1.2.3, got %s", tags[0].Name) + } +} + +func TestCheckTagProtection_Active(t *testing.T) { + reg := &httpmock.Registry{} + defer reg.Verify(t) + reg.Register( + httpmock.REST("GET", "repos/owner/repo/rulesets"), + httpmock.JSONResponse([]map[string]interface{}{ + {"id": 1, "name": "protect-tags", "target": "tag", "enforcement": "active"}, + }), + ) + + diags := checkTagProtection(api.NewClientFromHTTP(&http.Client{Transport: reg}), "github.com", "owner", "repo") + if len(diags) != 0 { + t.Errorf("expected no diagnostics when tag protection active, got: %v", diags) + } +} + +func TestCheckTagProtection_Missing(t *testing.T) { + reg := &httpmock.Registry{} + defer reg.Verify(t) + reg.Register( + httpmock.REST("GET", "repos/owner/repo/rulesets"), + httpmock.JSONResponse([]map[string]interface{}{ + {"id": 1, "name": "branch-protection", "target": "branch", "enforcement": "active"}, + }), + ) + + diags := checkTagProtection(api.NewClientFromHTTP(&http.Client{Transport: reg}), "github.com", "owner", "repo") + if len(diags) != 1 { + t.Fatalf("expected 1 diagnostic, got %d", len(diags)) + } + if !strings.Contains(diags[0].message, "tag protection") { + t.Errorf("expected tag protection warning, got: %s", diags[0].message) + } +} + +func TestCheckSecuritySettings_AllEnabled(t *testing.T) { + reg := &httpmock.Registry{} + defer reg.Verify(t) + reg.Register( + httpmock.REST("GET", "repos/owner/repo"), + httpmock.JSONResponse(map[string]interface{}{ + "security_and_analysis": map[string]interface{}{ + "secret_scanning": map[string]interface{}{"status": "enabled"}, + "secret_scanning_push_protection": map[string]interface{}{"status": "enabled"}, + }, + }), + ) + + skillsDir := t.TempDir() + + diags := checkSecuritySettings(api.NewClientFromHTTP(&http.Client{Transport: reg}), "github.com", "owner", "repo", skillsDir) + if len(diags) != 0 { + t.Errorf("expected no diagnostics when all security enabled, got %d: %v", len(diags), diags) + } +} + +func TestCheckSecuritySettings_NoneEnabled(t *testing.T) { + reg := &httpmock.Registry{} + defer reg.Verify(t) + reg.Register( + httpmock.REST("GET", "repos/owner/repo"), + httpmock.JSONResponse(map[string]interface{}{ + "security_and_analysis": map[string]interface{}{ + "secret_scanning": map[string]interface{}{"status": "disabled"}, + "secret_scanning_push_protection": map[string]interface{}{"status": "disabled"}, + }, + }), + ) + + skillsDir := t.TempDir() + + diags := checkSecuritySettings(api.NewClientFromHTTP(&http.Client{Transport: reg}), "github.com", "owner", "repo", skillsDir) + if len(diags) != 2 { + t.Errorf("expected 2 diagnostics (secret scanning + push protection), got %d: %v", len(diags), diags) + } + for _, d := range diags { + if d.severity != "warning" { + t.Errorf("secret scanning diagnostics should be warnings, got %q: %s", d.severity, d.message) + } + } +} + +func TestCheckSecuritySettings_WithCodeFiles(t *testing.T) { + reg := &httpmock.Registry{} + defer reg.Verify(t) + reg.Register( + httpmock.REST("GET", "repos/owner/repo"), + httpmock.JSONResponse(map[string]interface{}{ + "security_and_analysis": map[string]interface{}{ + "secret_scanning": map[string]interface{}{"status": "enabled"}, + "secret_scanning_push_protection": map[string]interface{}{"status": "enabled"}, + }, + }), + ) + reg.Register( + httpmock.REST("GET", "repos/owner/repo/code-scanning/alerts"), + httpmock.StatusStringResponse(404, "not found"), + ) + + skillsDir := t.TempDir() + scriptDir := filepath.Join(skillsDir, "my-skill", "scripts") + if err := os.MkdirAll(scriptDir, 0o755); err != nil { + t.Fatal(err) + } + if err := os.WriteFile(filepath.Join(scriptDir, "helper.sh"), []byte("#!/bin/bash"), 0o644); err != nil { + t.Fatal(err) + } + + diags := checkSecuritySettings(api.NewClientFromHTTP(&http.Client{Transport: reg}), "github.com", "owner", "repo", skillsDir) + hasCodeScanInfo := false + for _, d := range diags { + if strings.Contains(d.message, "code scanning") { + hasCodeScanInfo = true + if d.severity != "info" { + t.Errorf("code scanning suggestion should be info, got %q", d.severity) + } + } + } + if !hasCodeScanInfo { + t.Error("expected code scanning info when code files present") + } +} + +func TestCheckSecuritySettings_WithManifests(t *testing.T) { + reg := &httpmock.Registry{} + defer reg.Verify(t) + reg.Register( + httpmock.REST("GET", "repos/owner/repo"), + httpmock.JSONResponse(map[string]interface{}{ + "security_and_analysis": map[string]interface{}{ + "secret_scanning": map[string]interface{}{"status": "enabled"}, + "secret_scanning_push_protection": map[string]interface{}{"status": "enabled"}, + }, + }), + ) + reg.Register( + httpmock.REST("GET", "repos/owner/repo/vulnerability-alerts"), + httpmock.StatusStringResponse(404, "not found"), + ) + + skillsDir := t.TempDir() + skillDir := filepath.Join(skillsDir, "my-skill") + if err := os.MkdirAll(skillDir, 0o755); err != nil { + t.Fatal(err) + } + if err := os.WriteFile(filepath.Join(skillDir, "package.json"), []byte("{}"), 0o644); err != nil { + t.Fatal(err) + } + + diags := checkSecuritySettings(api.NewClientFromHTTP(&http.Client{Transport: reg}), "github.com", "owner", "repo", skillsDir) + hasDependabotInfo := false + for _, d := range diags { + if strings.Contains(d.message, "Dependabot") { + hasDependabotInfo = true + if d.severity != "info" { + t.Errorf("Dependabot suggestion should be info, got %q", d.severity) + } + } + } + if !hasDependabotInfo { + t.Error("expected Dependabot info when manifest files present") + } +} + +func TestDetectCodeAndManifests(t *testing.T) { + dir := t.TempDir() + + hasCode, hasManifests := detectCodeAndManifests(dir) + if hasCode || hasManifests { + t.Error("empty dir should have no code or manifests") + } + + if err := os.WriteFile(filepath.Join(dir, "run.sh"), []byte("#!/bin/bash"), 0o644); err != nil { + t.Fatal(err) + } + hasCode, hasManifests = detectCodeAndManifests(dir) + if !hasCode { + t.Error("should detect .sh as code") + } + if hasManifests { + t.Error("should not detect manifests") + } + + if err := os.WriteFile(filepath.Join(dir, "requirements.txt"), []byte("flask"), 0o644); err != nil { + t.Fatal(err) + } + hasCode, hasManifests = detectCodeAndManifests(dir) + if !hasCode || !hasManifests { + t.Error("should detect both code and manifests") + } +} + +func TestCheckInstalledSkillDirs_NotPresent(t *testing.T) { + dir := t.TempDir() + diags := checkInstalledSkillDirs(nil, dir) + if len(diags) != 0 { + t.Errorf("expected no diagnostics for empty dir, got %d", len(diags)) + } +} + +func TestCheckInstalledSkillDirs_PresentNotIgnored(t *testing.T) { + gitClient := testPublishGitClient(t, nil) + dir := gitClient.RepoDir + + installedDir := filepath.Join(dir, ".github", "skills", "some-skill") + if err := os.MkdirAll(installedDir, 0o755); err != nil { + t.Fatal(err) + } + + diags := checkInstalledSkillDirs(gitClient, dir) + if len(diags) == 0 { + t.Fatal("expected warning for unignored .github/skills/") + } + if diags[0].severity != "warning" { + t.Errorf("expected warning, got %q", diags[0].severity) + } + if !strings.Contains(diags[0].message, ".gitignore") { + t.Errorf("expected .gitignore mention, got: %s", diags[0].message) + } +} + +func TestCheckInstalledSkillDirs_PresentAndIgnored(t *testing.T) { + gitClient := testPublishGitClient(t, nil) + dir := gitClient.RepoDir + + installedDir := filepath.Join(dir, ".github", "skills", "some-skill") + if err := os.MkdirAll(installedDir, 0o755); err != nil { + t.Fatal(err) + } + + // Add .gitignore so git check-ignore recognises the path. + if err := os.WriteFile(filepath.Join(dir, ".gitignore"), []byte(".github/skills\n"), 0o644); err != nil { + t.Fatal(err) + } + runGit := func(args ...string) { + t.Helper() + cmd := exec.Command("git", append([]string{"-C", dir}, args...)...) + cmd.Env = append(os.Environ(), "GIT_CONFIG_NOSYSTEM=1", "HOME="+dir) + out, err := cmd.CombinedOutput() + require.NoError(t, err, "git %v: %s", args, out) + } + runGit("add", ".gitignore") + runGit("commit", "-m", "init") + + diags := checkInstalledSkillDirs(gitClient, dir) + if len(diags) != 0 { + t.Errorf("expected no diagnostics when gitignored, got %d: %v", len(diags), diags) + } +} + +func TestGenerateClaudePlugin(t *testing.T) { + dir := t.TempDir() + + for _, name := range []string{"git-commit", "code-review"} { + skillDir := filepath.Join(dir, "skills", name) + if err := os.MkdirAll(skillDir, 0o755); err != nil { + t.Fatal(err) + } + content := fmt.Sprintf("---\nname: %s\ndescription: A %s skill\nlicense: MIT\n---\nBody.\n", name, name) + if err := os.WriteFile(filepath.Join(skillDir, "SKILL.md"), []byte(content), 0o644); err != nil { + t.Fatal(err) + } + } + + diags := generateClaudePlugin(dir, []string{"git-commit", "code-review"}, "testowner", "testrepo") + + var generated int + for _, d := range diags { + if d.severity == "error" { + t.Errorf("unexpected error: %s", d.message) + } + if d.severity == "info" && strings.Contains(d.message, "generated") { + generated++ + } + } + if generated != 2 { + t.Errorf("expected 2 generated files, got %d", generated) + } + + pluginData, err := os.ReadFile(filepath.Join(dir, ".claude-plugin", "plugin.json")) + if err != nil { + t.Fatalf("plugin.json not created: %v", err) + } + var plugin claudePluginJSON + if err := json.Unmarshal(pluginData, &plugin); err != nil { + t.Fatalf("invalid plugin.json: %v", err) + } + if plugin.Name != "testrepo" { + t.Errorf("plugin.Name = %q, want %q", plugin.Name, "testrepo") + } + if plugin.License != "MIT" { + t.Errorf("plugin.License = %q, want %q", plugin.License, "MIT") + } + if plugin.Repository != "https://github.com/testowner/testrepo" { + t.Errorf("plugin.Repository = %q", plugin.Repository) + } + + marketData, err := os.ReadFile(filepath.Join(dir, ".claude-plugin", "marketplace.json")) + if err != nil { + t.Fatalf("marketplace.json not created: %v", err) + } + var marketplace claudeMarketplaceJSON + if err := json.Unmarshal(marketData, &marketplace); err != nil { + t.Fatalf("invalid marketplace.json: %v", err) + } + if marketplace.Name != "testrepo" { + t.Errorf("marketplace.Name = %q, want %q", marketplace.Name, "testrepo") + } + if len(marketplace.Plugins) != 1 || marketplace.Plugins[0].Source != "." { + t.Errorf("marketplace.Plugins = %+v", marketplace.Plugins) + } +} + +func TestGenerateClaudePlugin_SkipsExisting(t *testing.T) { + dir := t.TempDir() + + skillDir := filepath.Join(dir, "skills", "my-skill") + if err := os.MkdirAll(skillDir, 0o755); err != nil { + t.Fatal(err) + } + if err := os.WriteFile(filepath.Join(skillDir, "SKILL.md"), []byte("---\nname: my-skill\ndescription: test\n---\nBody.\n"), 0o644); err != nil { + t.Fatal(err) + } + + pluginDir := filepath.Join(dir, ".claude-plugin") + if err := os.MkdirAll(pluginDir, 0o755); err != nil { + t.Fatal(err) + } + if err := os.WriteFile(filepath.Join(pluginDir, "plugin.json"), []byte(`{"name":"existing"}`), 0o644); err != nil { + t.Fatal(err) + } + + diags := generateClaudePlugin(dir, []string{"my-skill"}, "owner", "repo") + + for _, d := range diags { + if d.severity == "error" { + t.Errorf("unexpected error: %s", d.message) + } + if strings.Contains(d.message, "generated") { + t.Error("should not regenerate existing plugin.json") + } + } +} + +func TestDetectGitHubRemote(t *testing.T) { + gitClient := testPublishGitClient(t, map[string]string{ + "origin": "https://github.com/myorg/myrepo.git", + }) + + owner, repo := detectGitHubRemote(gitClient) + if owner != "myorg" || repo != "myrepo" { + t.Errorf("expected myorg/myrepo, got %s/%s", owner, repo) + } +} + +func TestDetectGitHubRemote_Fallback(t *testing.T) { + gitClient := testPublishGitClient(t, map[string]string{ + "origin": "https://gitlab.com/foo/bar.git", + "upstream": "git@github.com:org/repo.git", + }) + + owner, repo := detectGitHubRemote(gitClient) + if owner != "org" || repo != "repo" { + t.Errorf("expected org/repo, got %s/%s", owner, repo) + } +} + +func TestDetectGitHubRemote_NoGitHub(t *testing.T) { + gitClient := testPublishGitClient(t, map[string]string{ + "origin": "https://gitlab.com/foo/bar.git", + }) + + owner, repo := detectGitHubRemote(gitClient) + if owner != "" || repo != "" { + t.Errorf("expected empty, got %s/%s", owner, repo) + } +} + +func TestPublishCmd_RunFHook(t *testing.T) { + ios, _, _, _ := iostreams.Test() + f := stubFactory(ios) + + var capturedOpts *publishOptions + cmd := NewCmdPublish(&f, func(opts *publishOptions) error { + capturedOpts = opts + return nil + }) + + cmd.SetArgs([]string{"./my-skills", "--dry-run", "--fix", "--tag", "v1.0.0"}) + cmd.SetOut(&bytes.Buffer{}) + cmd.SetErr(&bytes.Buffer{}) + + err := cmd.Execute() + if err != nil { + t.Fatalf("unexpected error: %v", err) + } + + if capturedOpts == nil { + t.Fatal("runF was not called") + } + if capturedOpts.Dir != "./my-skills" { + t.Errorf("Dir = %q, want %q", capturedOpts.Dir, "./my-skills") + } + if !capturedOpts.DryRun { + t.Error("expected DryRun to be true") + } + if !capturedOpts.Fix { + t.Error("expected Fix to be true") + } + if capturedOpts.Tag != "v1.0.0" { + t.Errorf("Tag = %q, want %q", capturedOpts.Tag, "v1.0.0") + } +} + +// stubFactory creates a minimal cmdutil.Factory for tests. +func stubFactory(ios *iostreams.IOStreams) cmdutil.Factory { + return cmdutil.Factory{ + IOStreams: ios, + } +} diff --git a/pkg/cmd/skills/search/search.go b/pkg/cmd/skills/search/search.go new file mode 100644 index 000000000..0d7e39043 --- /dev/null +++ b/pkg/cmd/skills/search/search.go @@ -0,0 +1,873 @@ +package search + +import ( + "errors" + "fmt" + "math" + "net/http" + "net/url" + "os" + "os/exec" + "sort" + "strings" + "sync" + + "github.com/MakeNowJust/heredoc" + "github.com/cli/cli/v2/api" + "github.com/cli/cli/v2/internal/gh" + "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/tableprinter" + "github.com/cli/cli/v2/internal/text" + "github.com/cli/cli/v2/pkg/cmdutil" + "github.com/cli/cli/v2/pkg/iostreams" + "github.com/spf13/cobra" +) + +const ( + defaultLimit = 15 + maxResults = 1000 // GitHub Code Search API hard limit + + // searchPageSize is the number of raw results to request from the + // GitHub Search API per call (max allowed). + searchPageSize = 100 +) + +// SkillSearchFields defines the set of fields available for --json output. +var SkillSearchFields = []string{ + "repo", + "skillName", + "description", + "stars", + "path", +} + +type searchOptions struct { + IO *iostreams.IOStreams + HttpClient func() (*http.Client, error) + Config func() (gh.Config, error) + Prompter prompter.Prompter + Executable string // path to the current gh binary for install subprocess + Exporter cmdutil.Exporter + + // User inputs + Query string + Owner string // optional: scope results to a specific GitHub owner + Page int + Limit int +} + +// NewCmdSearch creates the "skills search" command. +func NewCmdSearch(f *cmdutil.Factory, runF func(*searchOptions) error) *cobra.Command { + opts := &searchOptions{ + IO: f.IOStreams, + HttpClient: f.HttpClient, + Config: f.Config, + Prompter: f.Prompter, + Executable: f.Executable(), + } + + cmd := &cobra.Command{ + Use: "search ", + Short: "Search for skills across GitHub", + Long: heredoc.Doc(` + Search across all public GitHub repositories for skills matching a keyword. + + Uses the GitHub Code Search API to find SKILL.md files whose name or + description matches the query term. + + Results are ranked by relevance: skills whose name contains the query + term appear first. + + Use --owner to scope results to a specific GitHub user or organization. + + In interactive mode, you can select skills from the results to install + directly. + `), + Example: heredoc.Doc(` + # Search for skills related to terraform + $ gh skills search terraform + + # Search for skills from a specific owner + $ gh skills search terraform --owner hashicorp + + # View the second page of results + $ gh skills search terraform --page 2 + + # Limit results to 5 + $ gh skills search terraform --limit 5 + `), + Args: cmdutil.MinimumArgs(1, "cannot search: query argument required"), + RunE: func(c *cobra.Command, args []string) error { + opts.Query = strings.Join(args, " ") + + if len(strings.TrimSpace(opts.Query)) < 2 { + return cmdutil.FlagErrorf("search query must be at least 2 characters") + } + + if opts.Page < 1 { + return cmdutil.FlagErrorf("invalid page number: %d", opts.Page) + } + + if opts.Limit < 1 { + return cmdutil.FlagErrorf("invalid limit: %d", opts.Limit) + } + + opts.Owner = strings.TrimSpace(opts.Owner) + if opts.Owner != "" && !couldBeOwner(opts.Owner) { + return cmdutil.FlagErrorf("invalid owner %q: must be a valid GitHub username or organization", opts.Owner) + } + + if runF != nil { + return runF(opts) + } + return searchRun(opts) + }, + } + + cmd.Flags().IntVar(&opts.Page, "page", 1, "Page number of results to fetch") + cmd.Flags().IntVarP(&opts.Limit, "limit", "L", defaultLimit, "Maximum number of results per page") + cmd.Flags().StringVar(&opts.Owner, "owner", "", "Filter results to a specific GitHub user or organization") + cmdutil.AddJSONFlags(cmd, &opts.Exporter, SkillSearchFields) + + return cmd +} + +// codeSearchResult represents the GitHub Code Search API response. +type codeSearchResult struct { + TotalCount int `json:"total_count"` + IncompleteResults bool `json:"incomplete_results"` + Items []codeSearchItem `json:"items"` +} + +// codeSearchItem represents a single code search hit. +type codeSearchItem struct { + Name string `json:"name"` + Path string `json:"path"` + SHA string `json:"sha"` + Repository codeSearchRepository `json:"repository"` +} + +// codeSearchRepository is the repo info embedded in a code search hit. +type codeSearchRepository struct { + FullName string `json:"full_name"` +} + +// skillResult is a deduplicated search result. +type skillResult struct { + Repo string + Owner string // parsed from Repo + RepoName string // parsed from Repo + SkillName string + Description string + Path string // original file path (e.g. skills/terraform/SKILL.md) + BlobSHA string + Stars int // repository stargazer count +} + +// ExportData implements cmdutil.exportable for --json output. +func (s skillResult) ExportData(fields []string) map[string]interface{} { + data := map[string]interface{}{} + for _, f := range fields { + switch f { + case "repo": + data[f] = s.Repo + case "skillName": + data[f] = s.SkillName + case "description": + data[f] = s.Description + case "stars": + data[f] = s.Stars + case "path": + data[f] = s.Path + } + } + return data +} + +func searchRun(opts *searchOptions) error { + httpClient, err := opts.HttpClient() + if err != nil { + return err + } + + apiClient := api.NewClientFromHTTP(httpClient) + + cfg, err := opts.Config() + if err != nil { + return err + } + host, _ := cfg.Authentication().DefaultHost() + + opts.IO.StartProgressIndicatorWithLabel("Searching for skills") + + skills, err := searchByKeyword(apiClient, host, opts.Query, opts.Owner, opts.Page, opts.Limit) + if err != nil { + opts.IO.StopProgressIndicator() + return err + } + + if len(skills) == 0 { + opts.IO.StopProgressIndicator() + return noResults(opts, noResultsMessage(opts)) + } + + // Pre-rank before expensive enrichment, then truncate working set. + rankByRelevance(skills, opts.Query) + skills = truncateForProcessing(skills, opts.Page, opts.Limit) + + enrichSkills(apiClient, host, skills) + opts.IO.StopProgressIndicator() + + // Filter out noise and re-rank with enriched data (descriptions, stars). + skills = filterByRelevance(skills, opts.Query) + if len(skills) == 0 { + return noResults(opts, noResultsMessage(opts)) + } + rankByRelevance(skills, opts.Query) + + // Collapse duplicate skill names across repos, keeping up to 3 + // top-ranked instances of each. Prevents aggregator repos + // (which copy popular skills) from flooding results. + skills = deduplicateByName(skills) + + // Paginate to the requested page window. + var totalPages int + skills, totalPages = paginate(skills, opts.Page, opts.Limit) + if len(skills) == 0 { + msg := fmt.Sprintf("no skills found on page %d for query %q", opts.Page, opts.Query) + if opts.Owner != "" { + msg = fmt.Sprintf("no skills found on page %d for query %q from owner %q", opts.Page, opts.Query, opts.Owner) + } + return noResults(opts, msg) + } + + return renderResults(opts, skills, totalPages) +} + +// noResultsMessage returns an appropriate "no results" message. +func noResultsMessage(opts *searchOptions) string { + if opts.Owner != "" { + return fmt.Sprintf("no skills found matching %q from owner %q", opts.Query, opts.Owner) + } + return fmt.Sprintf("no skills found matching %q", opts.Query) +} + +// searchByKeyword runs parallel searches: content match, path match, owner +// match (for single-word queries), and (for multi-word queries) a hyphenated +// content match to catch skill names like "mcp-apps" when the user types +// "mcp apps". When owner is non-empty, all queries are scoped to that +// GitHub user/org via user: and the implicit owner search is skipped. +func searchByKeyword(client *api.Client, host, queryTerm, owner string, page, limit int) ([]skillResult, error) { + ownerScope := "" + if owner != "" { + ownerScope = " user:" + owner + } + + primaryQ := fmt.Sprintf("filename:SKILL.md %s%s", queryTerm, ownerScope) + pathTerm := strings.ReplaceAll(queryTerm, " ", "-") + pathQ := fmt.Sprintf("filename:SKILL.md path:%s%s", pathTerm, ownerScope) + + var ( + primaryItems []codeSearchItem + primaryErr error + pathResult *codeSearchResult + pathErr error + ownerResult *codeSearchResult + ownerErr error + hyphenResult *codeSearchResult + hyphenErr error + ) + + hasSpaces := strings.Contains(queryTerm, " ") + + var wg sync.WaitGroup + + wg.Add(1) + go func() { + defer wg.Done() + pathResult, pathErr = executeSearch(client, host, pathQ, 1, searchPageSize) + }() + + // When no explicit --owner is set and the query looks like it could be a + // GitHub username, fire an additional user: search to discover + // skills published by that org. Results compete on the same footing as + // everything else (no scoring boost). + if owner == "" && couldBeOwner(queryTerm) { + ownerQ := fmt.Sprintf("filename:SKILL.md user:%s", queryTerm) + wg.Add(1) + go func() { + defer wg.Done() + ownerResult, ownerErr = executeSearch(client, host, ownerQ, 1, searchPageSize) + }() + } + + // When the query has spaces (e.g. "mcp apps"), run an additional content + // search with the hyphenated form ("mcp-apps") so we don't miss skills + // whose names use hyphens as word separators. + if hasSpaces { + hyphenQ := fmt.Sprintf("filename:SKILL.md %s%s", pathTerm, ownerScope) + wg.Add(1) + go func() { + defer wg.Done() + hyphenResult, hyphenErr = executeSearch(client, host, hyphenQ, 1, searchPageSize) + }() + } + + // Primary content search runs on the main goroutine. + primaryItems, _, primaryErr = fetchPrimaryPages(client, host, primaryQ, page, limit) + wg.Wait() + + if primaryErr != nil { + return nil, primaryErr + } + + // Merge: path-matched → hyphen-matched → owner-matched → primary content. + var merged []codeSearchItem + + if pathErr == nil && pathResult != nil { + merged = append(merged, pathResult.Items...) + } + if hasSpaces && hyphenErr == nil && hyphenResult != nil { + merged = append(merged, hyphenResult.Items...) + } + if ownerErr == nil && ownerResult != nil { + merged = append(merged, ownerResult.Items...) + } + merged = append(merged, primaryItems...) + + return deduplicateResults(merged), nil +} + +// noResults returns an empty JSON array for exporters or a no-results error. +func noResults(opts *searchOptions, msg string) error { + if opts.Exporter != nil { + return opts.Exporter.Write(opts.IO, []skillResult{}) + } + return cmdutil.NewNoResultsError(msg) +} + +// truncateForProcessing caps the working set before expensive enrichment. +// Each skill in the working set triggers a blob fetch (description) and +// potentially a repo fetch (stars), so keeping this small matters for +// performance. Pre-ranking ensures the best candidates are at the top. +func truncateForProcessing(skills []skillResult, page, limit int) []skillResult { + maxToProcess := page * limit * 3 + if maxToProcess < limit*3 { + maxToProcess = limit * 3 + } + if len(skills) > maxToProcess { + return skills[:maxToProcess] + } + return skills +} + +// enrichSkills fetches descriptions and star counts concurrently. +func enrichSkills(client *api.Client, host string, skills []skillResult) { + var wg sync.WaitGroup + wg.Add(2) + go func() { + defer wg.Done() + fetchDescriptions(client, host, skills) + }() + go func() { + defer wg.Done() + fetchRepoStars(client, host, skills) + }() + wg.Wait() +} + +// paginate slices results to the requested page window. +func paginate(skills []skillResult, page, limit int) ([]skillResult, int) { + total := len(skills) + totalPages := (total + limit - 1) / limit + start := (page - 1) * limit + if start >= total { + return nil, totalPages + } + end := start + limit + if end > total { + end = total + } + return skills[start:end], totalPages +} + +// deduplicateByName caps the number of results with the same skill name. +// Since results are pre-sorted by relevance score, the first occurrences +// are the best instances. This prevents aggregator repos (which copy +// popular skills verbatim) from flooding results while still showing +// a few alternative sources. +func deduplicateByName(skills []skillResult) []skillResult { + const maxPerName = 3 + counts := make(map[string]int) + var result []skillResult + for _, s := range skills { + key := strings.ToLower(s.SkillName) + if counts[key] >= maxPerName { + continue + } + counts[key]++ + result = append(result, s) + } + return result +} + +// renderResults handles all output modes: JSON, interactive picker, or table. +func renderResults(opts *searchOptions, skills []skillResult, totalPages int) error { + if opts.Exporter != nil { + return opts.Exporter.Write(opts.IO, skills) + } + + cs := opts.IO.ColorScheme() + header := fmt.Sprintf("\n%s Showing %s matching %q", + cs.SuccessIcon(), + pluralize(len(skills), "skill"), + opts.Query, + ) + if totalPages > 1 { + header += fmt.Sprintf(" (page %d/%d)", opts.Page, totalPages) + } + + if opts.IO.CanPrompt() { + fmt.Fprintln(opts.IO.ErrOut, header) + if opts.Page < totalPages { + fmt.Fprintf(opts.IO.ErrOut, "Use --page %d for more results.\n", opts.Page+1) + } + return promptInstall(opts, skills) + } + + // Non-interactive mode: render table. + if opts.IO.IsStdoutTTY() { + fmt.Fprintln(opts.IO.Out, header) + fmt.Fprintln(opts.IO.Out) + } + + if err := renderTable(opts.IO, skills); err != nil { + return err + } + + if opts.IO.IsStdoutTTY() && opts.Page < totalPages { + fmt.Fprintf(opts.IO.ErrOut, "\nUse --page %d for more results.\n", opts.Page+1) + } + + return nil +} + +// renderTable outputs a formatted table of skill results. +func renderTable(io *iostreams.IOStreams, skills []skillResult) error { + isTTY := io.IsStdoutTTY() + tw := io.TerminalWidth() + descWidth := tw - 70 + if descWidth < 20 { + descWidth = 20 + } + + table := tableprinter.New(io, tableprinter.WithHeader("REPOSITORY", "SKILL", "DESCRIPTION", "STARS")) + for _, s := range skills { + table.AddField(s.Repo) + table.AddField(s.SkillName) + desc := s.Description + if isTTY { + desc = text.Truncate(descWidth, desc) + } + table.AddField(desc) + table.AddField(formatStars(s.Stars)) + table.EndRow() + } + return table.Render() +} + +// promptInstall shows a multi-select picker for the user to choose skills +// to install from the search results, then runs the install command for each. +func promptInstall(opts *searchOptions, skills []skillResult) error { + fmt.Fprintln(opts.IO.ErrOut) + + cs := opts.IO.ColorScheme() + + // Reserve space for the checkbox UI prefix ("[ ] ") and the description + // indent ("\n " = 7 chars), then use the remaining terminal width. + tw := opts.IO.TerminalWidth() + descWidth := tw - 11 + if descWidth < 30 { + descWidth = 30 + } + + options := make([]string, len(skills)) + for i, s := range skills { + starStr := "" + if s.Stars > 0 { + starStr = " " + cs.Gray("★ "+formatStars(s.Stars)) + } + descStr := "" + if s.Description != "" { + desc := collapseWhitespace(s.Description) + descStr = "\n " + cs.Gray(text.Truncate(descWidth, desc)) + } + options[i] = s.SkillName + " " + cs.Gray(s.Repo) + starStr + descStr + } + + indices, err := opts.Prompter.MultiSelect( + "Select skills to install (press Enter to skip):", + nil, + options, + ) + if err != nil { + return err + } + + if len(indices) == 0 { + return nil + } + + // Prompt for target agent host (once for all selected skills) + hostNames := registry.AgentNames() + hostIdx, err := opts.Prompter.Select("Select target agent:", "", hostNames) + if err != nil { + return err + } + host := registry.Agents[hostIdx] + + // Prompt for installation scope + scopeIdx, err := opts.Prompter.Select("Installation scope:", "", registry.ScopeLabels("")) + if err != nil { + return err + } + scope := string(registry.ScopeProject) + if scopeIdx == 1 { + scope = string(registry.ScopeUser) + } + + for _, idx := range indices { + s := skills[idx] + fmt.Fprintf(opts.IO.ErrOut, "\n%s Installing %s from %s...\n", + cs.Blue("::"), s.SkillName, s.Repo) + + //nolint:gosec // arguments are from user-selected search results, not arbitrary input + cmd := exec.Command(opts.Executable, "skills", "install", s.Repo, s.SkillName, + "--agent", host.ID, "--scope", scope) + cmd.Stdin = os.Stdin + cmd.Stdout = opts.IO.Out + cmd.Stderr = opts.IO.ErrOut + if err := cmd.Run(); err != nil { + fmt.Fprintf(opts.IO.ErrOut, "%s Failed to install %s from %s: %s\n", + cs.Red("!"), s.SkillName, s.Repo, err) + } + } + + return nil +} + +// relevanceScore computes a numeric ranking score for a search result. +// Higher scores rank first. Signals (in priority order): +// - Exact skill name match (10 000 points) +// - Partial skill name match (1 000 points) +// - Description contains query (100 points) +// - Repository stars (logarithmic bonus, up to ~700 points) +func relevanceScore(s skillResult, query string) int { + term := strings.ToLower(query) + termHyphen := strings.ReplaceAll(term, " ", "-") + score := 0 + + // Name match. Normalize spaces to hyphens since skill directory names + // use hyphens as word separators (e.g. query "mcp apps" → "mcp-apps"). + skillLower := strings.ToLower(s.SkillName) + if skillLower == term || skillLower == termHyphen { + score += 10_000 + } else if strings.Contains(skillLower, term) || strings.Contains(skillLower, termHyphen) { + score += 1_000 + } + + // Description match. + if strings.Contains(strings.ToLower(s.Description), term) { + score += 100 + } + + // Stars bonus: use log₁₀ scaling so popular repos rank higher without + // completely drowning out less-popular but more relevant results. + if s.Stars > 0 { + score += int(math.Log10(float64(s.Stars)) * 150) + } + + return score +} + +// filterByRelevance removes results that are not meaningfully related to +// the query. A result is kept if the query term appears in the skill name, +// the YAML description, or the repository owner or name. +func filterByRelevance(skills []skillResult, query string) []skillResult { + queryTerm := strings.ToLower(query) + termHyphen := strings.ReplaceAll(queryTerm, " ", "-") + + filtered := skills[:0] // reuse backing array + for _, s := range skills { + nameLower := strings.ToLower(s.SkillName) + descLower := strings.ToLower(s.Description) + ownerLower := strings.ToLower(s.Owner) + repoLower := strings.ToLower(s.RepoName) + + if strings.Contains(nameLower, queryTerm) || + strings.Contains(nameLower, termHyphen) || + strings.Contains(descLower, queryTerm) || + strings.Contains(ownerLower, queryTerm) || + strings.Contains(repoLower, queryTerm) { + filtered = append(filtered, s) + } + } + return filtered +} + +// rankByRelevance sorts results by multi-signal score, highest first. +func rankByRelevance(skills []skillResult, query string) { + sort.SliceStable(skills, func(i, j int) bool { + return relevanceScore(skills[i], query) > relevanceScore(skills[j], query) + }) +} + +// couldBeOwner returns true if s looks like a valid GitHub username/org. +// GitHub usernames: 1-39 chars, alphanumeric or hyphen, no leading/trailing hyphens. +func couldBeOwner(s string) bool { + if len(s) == 0 || len(s) > 39 { + return false + } + for i, c := range s { + switch { + case c >= 'a' && c <= 'z', c >= 'A' && c <= 'Z', c >= '0' && c <= '9': + continue + case c == '-': + if i == 0 || i == len(s)-1 { + return false + } + default: + return false + } + } + return true +} + +// isRateLimitError checks whether err is a GitHub API rate-limit response. +// Per GitHub docs, a rate limit is indicated by: +// - HTTP 429 (always a rate limit) +// - HTTP 403 with x-ratelimit-remaining: 0 (primary rate limit) +// - HTTP 403 with a retry-after header (secondary rate limit) +func isRateLimitError(err error) bool { + var httpErr api.HTTPError + if !errors.As(err, &httpErr) { + return false + } + if httpErr.StatusCode == 429 { + return true + } + if httpErr.StatusCode == 403 { + if httpErr.Headers.Get("x-ratelimit-remaining") == "0" { + return true + } + if httpErr.Headers.Get("retry-after") != "" { + return true + } + } + return false +} + +// rateLimitErrorMessage returns a user-friendly message for rate-limit errors. +const rateLimitErrorMessage = "GitHub API rate limit exceeded. Please wait a minute and try again." + +// executeSearch performs a single GitHub Code Search API call. +func executeSearch(client *api.Client, host, query string, page, pageSize int) (*codeSearchResult, error) { + apiPath := fmt.Sprintf("search/code?q=%s&per_page=%d&page=%d", + url.QueryEscape(query), pageSize, page) + var result codeSearchResult + err := client.REST(host, "GET", apiPath, nil, &result) + if err != nil && isRateLimitError(err) { + return nil, fmt.Errorf("%s", rateLimitErrorMessage) + } + return &result, err +} + +// fetchPrimaryPages fetches enough API pages from GitHub Code Search to +// cover the requested display page, accounting for filtering losses. +func fetchPrimaryPages(client *api.Client, host, query string, displayPage, displayLimit int) ([]codeSearchItem, int, error) { + // Over-fetch to account for deduplication + filtering losses. + // The Code Search API is rate-limited at 10 req/min, so we keep + // page fetching conservative. Two pages (200 results) provides a + // good buffer for typical filter rates while staying well within + // the rate-limit budget. + needed := displayPage * displayLimit * 3 + numPages := (needed + searchPageSize - 1) / searchPageSize + if numPages < 1 { + numPages = 1 + } + maxAPIPages := maxResults / searchPageSize + if numPages > maxAPIPages { + numPages = maxAPIPages + } + + var allItems []codeSearchItem + var totalCount int + for p := 1; p <= numPages; p++ { + result, err := executeSearch(client, host, query, p, searchPageSize) + if err != nil { + if p == 1 { + return nil, 0, err + } + break // partial results from earlier pages are OK + } + allItems = append(allItems, result.Items...) + totalCount = result.TotalCount + if len(result.Items) < searchPageSize { + break // no more results available + } + } + return allItems, totalCount, nil +} + +// deduplicateResults extracts unique (repo, skill name) pairs from code search hits. +func deduplicateResults(items []codeSearchItem) []skillResult { + seen := make(map[string]struct{}) + var results []skillResult + + for _, item := range items { + skillName := extractSkillName(item.Path) + if skillName == "" { + continue + } + key := item.Repository.FullName + "/" + skillName + if _, ok := seen[key]; ok { + continue + } + seen[key] = struct{}{} + + owner, repoName := splitRepo(item.Repository.FullName) + results = append(results, skillResult{ + Repo: item.Repository.FullName, + Owner: owner, + RepoName: repoName, + SkillName: skillName, + Path: item.Path, + BlobSHA: item.SHA, + }) + } + + return results +} + +// splitRepo splits "owner/repo" into its components. +func splitRepo(fullName string) (string, string) { + parts := strings.SplitN(fullName, "/", 2) + if len(parts) != 2 { + return fullName, "" + } + return parts[0], parts[1] +} + +// fetchDescriptions fetches SKILL.md frontmatter descriptions concurrently +// for all search results. Each result may come from a different repo. +func fetchDescriptions(client *api.Client, host string, skills []skillResult) { + const maxWorkers = 10 + sem := make(chan struct{}, maxWorkers) + var wg sync.WaitGroup + var mu sync.Mutex + + for i := range skills { + if skills[i].BlobSHA == "" { + continue + } + wg.Add(1) + go func(idx int) { + defer wg.Done() + sem <- struct{}{} + defer func() { <-sem }() + + content, err := discovery.FetchBlob(client, host, skills[idx].Owner, skills[idx].RepoName, skills[idx].BlobSHA) + if err != nil { + return + } + result, err := frontmatter.Parse(content) + if err != nil { + return + } + + mu.Lock() + skills[idx].Description = result.Metadata.Description + mu.Unlock() + }(i) + } + wg.Wait() +} + +// extractSkillName derives the skill name from a SKILL.md path, but only if +// the path matches a known skill convention (skills/*, skills/scope/*, root-level, +// or plugins/*/skills/*). Returns empty string for non-conforming paths. +func extractSkillName(filePath string) string { + return discovery.MatchesSkillPath(filePath) +} + +func pluralize(count int, singular string) string { + if count == 1 { + return fmt.Sprintf("%d %s", count, singular) + } + return fmt.Sprintf("%d %ss", count, singular) +} + +// collapseWhitespace replaces runs of whitespace (newlines, tabs, etc.) +// with a single space. +func collapseWhitespace(s string) string { + fields := strings.Fields(s) + return strings.Join(fields, " ") +} + +// formatStars formats a star count for display (e.g. 1700 → "1.7k"). +func formatStars(n int) string { + if n >= 1000 { + return fmt.Sprintf("%.1fk", float64(n)/1000) + } + return fmt.Sprintf("%d", n) +} + +// repoInfo holds the subset of repository metadata we fetch for ranking. +type repoInfo struct { + StargazersCount int `json:"stargazers_count"` +} + +// fetchRepoStars fetches stargazer counts for each unique repository in +// the result set, using bounded concurrency. +func fetchRepoStars(client *api.Client, host string, skills []skillResult) { + const maxWorkers = 10 + sem := make(chan struct{}, maxWorkers) + var wg sync.WaitGroup + var mu sync.Mutex + + repoStars := make(map[string]int) + seen := make(map[string]bool) + + for _, s := range skills { + if seen[s.Repo] { + continue + } + seen[s.Repo] = true + + wg.Add(1) + go func(owner, repo, fullName string) { + defer wg.Done() + sem <- struct{}{} + defer func() { <-sem }() + + apiPath := fmt.Sprintf("repos/%s/%s", owner, repo) + var info repoInfo + if err := client.REST(host, "GET", apiPath, nil, &info); err != nil { + return + } + mu.Lock() + repoStars[fullName] = info.StargazersCount + mu.Unlock() + }(s.Owner, s.RepoName, s.Repo) + } + wg.Wait() + + for i := range skills { + if stars, ok := repoStars[skills[i].Repo]; ok { + skills[i].Stars = stars + } + } +} diff --git a/pkg/cmd/skills/search/search_test.go b/pkg/cmd/skills/search/search_test.go new file mode 100644 index 000000000..db266f460 --- /dev/null +++ b/pkg/cmd/skills/search/search_test.go @@ -0,0 +1,423 @@ +package search + +import ( + "net/http" + "testing" + + "github.com/cli/cli/v2/internal/config" + "github.com/cli/cli/v2/internal/gh" + "github.com/cli/cli/v2/pkg/cmdutil" + "github.com/cli/cli/v2/pkg/httpmock" + "github.com/cli/cli/v2/pkg/iostreams" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +func TestNewCmdSearch(t *testing.T) { + tests := []struct { + name string + args string + wantOpts searchOptions + wantErr string + }{ + { + name: "query argument", + args: "terraform", + wantOpts: searchOptions{Query: "terraform", Page: 1, Limit: defaultLimit}, + }, + { + name: "with page flag", + args: "terraform --page 3", + wantOpts: searchOptions{Query: "terraform", Page: 3, Limit: defaultLimit}, + }, + { + name: "with limit flag", + args: "terraform --limit 5", + wantOpts: searchOptions{Query: "terraform", Page: 1, Limit: 5}, + }, + { + name: "with limit short flag", + args: "terraform -L 10", + wantOpts: searchOptions{Query: "terraform", Page: 1, Limit: 10}, + }, + { + name: "with owner flag", + args: "terraform --owner hashicorp", + wantOpts: searchOptions{Query: "terraform", Owner: "hashicorp", Page: 1, Limit: defaultLimit}, + }, + { + name: "no arguments", + args: "", + wantErr: "cannot search: query argument required", + }, + { + name: "invalid page", + args: "terraform --page 0", + wantErr: "invalid page number: 0", + }, + { + name: "query too short", + args: "a", + wantErr: "search query must be at least 2 characters", + }, + { + name: "query too short single char", + args: "x", + wantErr: "search query must be at least 2 characters", + }, + { + name: "invalid limit zero", + args: "terraform --limit 0", + wantErr: "invalid limit: 0", + }, + { + name: "invalid limit negative", + args: "terraform --limit -1", + wantErr: "invalid limit: -1", + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + f := &cmdutil.Factory{} + var gotOpts *searchOptions + cmd := NewCmdSearch(f, func(opts *searchOptions) error { + gotOpts = opts + return nil + }) + + argv := []string{} + if tt.args != "" { + for _, part := range splitOnSpaces(tt.args) { + if part != "" { + argv = append(argv, part) + } + } + } + cmd.SetArgs(argv) + cmd.SetOut(&discardWriter{}) + cmd.SetErr(&discardWriter{}) + + _, err := cmd.ExecuteC() + if tt.wantErr != "" { + assert.Error(t, err) + assert.Contains(t, err.Error(), tt.wantErr) + return + } + + require.NoError(t, err) + assert.Equal(t, tt.wantOpts.Query, gotOpts.Query) + assert.Equal(t, tt.wantOpts.Owner, gotOpts.Owner) + assert.Equal(t, tt.wantOpts.Page, gotOpts.Page) + assert.Equal(t, tt.wantOpts.Limit, gotOpts.Limit) + }) + } +} + +func TestSearchRun(t *testing.T) { + const emptyCodeResponse = `{"total_count": 0, "incomplete_results": false, "items": []}` + + // stubKeywordSearch registers the HTTP stubs needed for a keyword search. + // searchByKeyword fires up to 3 concurrent search/code requests (path, + // owner, primary). Stubs are one-shot in httpmock, so we register one + // per request. + stubKeywordSearch := func(reg *httpmock.Registry, codeResponse string) { + for range 3 { + reg.Register( + httpmock.REST("GET", "search/code"), + httpmock.StringResponse(codeResponse), + ) + } + } + + tests := []struct { + name string + opts *searchOptions + tty bool + httpStubs func(*httpmock.Registry) + wantStdout string + wantStderr string + wantErr string + }{ + { + name: "displays results in non-TTY", + tty: false, + opts: &searchOptions{Query: "terraform", Page: 1, Limit: defaultLimit}, + httpStubs: func(reg *httpmock.Registry) { + stubKeywordSearch(reg, `{"total_count": 1, "incomplete_results": false, "items": [{"name": "SKILL.md", "path": "skills/terraform/SKILL.md", "repository": {"full_name": "github/awesome-skills"}}]}`) + }, + wantStdout: "github/awesome-skills\tterraform\t\t0\n", + }, + { + name: "deduplicates results", + tty: false, + opts: &searchOptions{Query: "terraform", Page: 1, Limit: defaultLimit}, + httpStubs: func(reg *httpmock.Registry) { + stubKeywordSearch(reg, `{"total_count": 3, "incomplete_results": false, "items": [{"name": "SKILL.md", "path": "skills/terraform/SKILL.md", "repository": {"full_name": "github/awesome-skills"}}, {"name": "SKILL.md", "path": "skills/terraform/SKILL.md", "repository": {"full_name": "github/awesome-skills"}}, {"name": "SKILL.md", "path": "skills/terraform-aws/SKILL.md", "repository": {"full_name": "github/awesome-skills"}}]}`) + }, + wantStdout: "github/awesome-skills\tterraform\t\t0\ngithub/awesome-skills\tterraform-aws\t\t0\n", + }, + { + name: "no results", + tty: true, + opts: &searchOptions{Query: "nonexistent", Page: 1, Limit: defaultLimit}, + httpStubs: func(reg *httpmock.Registry) { + stubKeywordSearch(reg, emptyCodeResponse) + }, + wantErr: `no skills found matching "nonexistent"`, + }, + { + name: "nested skill path", + tty: false, + opts: &searchOptions{Query: "my-skill", Page: 1, Limit: defaultLimit}, + httpStubs: func(reg *httpmock.Registry) { + stubKeywordSearch(reg, `{"total_count": 1, "incomplete_results": false, "items": [{"name": "SKILL.md", "path": "skills/author/my-skill/SKILL.md", "repository": {"full_name": "org/repo"}}]}`) + }, + wantStdout: "org/repo\tmy-skill\t\t0\n", + }, + { + name: "ranks name-matching results first", + tty: false, + opts: &searchOptions{Query: "terraform", Page: 1, Limit: defaultLimit}, + httpStubs: func(reg *httpmock.Registry) { + stubKeywordSearch(reg, `{"total_count": 3, "incomplete_results": false, "items": [ + {"name": "SKILL.md", "path": "skills/terraform-deploy/SKILL.md", "repository": {"full_name": "org/repo1"}}, + {"name": "SKILL.md", "path": "skills/terraform-plan/SKILL.md", "repository": {"full_name": "org/repo2"}}, + {"name": "SKILL.md", "path": "skills/terraform/SKILL.md", "repository": {"full_name": "org/repo3"}} + ]}`) + }, + // exact name match "terraform" first, then partial matches alphabetically by score + wantStdout: "org/repo3\tterraform\t\t0\norg/repo1\tterraform-deploy\t\t0\norg/repo2\tterraform-plan\t\t0\n", + }, + { + name: "caps total pages at 1000-result limit", + tty: false, + opts: &searchOptions{Query: "terraform", Page: 1, Limit: defaultLimit}, + httpStubs: func(reg *httpmock.Registry) { + stubKeywordSearch(reg, `{"total_count": 5000, "incomplete_results": false, "items": [{"name": "SKILL.md", "path": "skills/terraform/SKILL.md", "repository": {"full_name": "org/repo"}}]}`) + }, + // In non-TTY mode, no header or pagination text is shown + wantStdout: "org/repo\tterraform\t\t0\n", + }, + { + name: "page beyond available results", + tty: false, + opts: &searchOptions{Query: "terraform", Page: 999, Limit: defaultLimit}, + httpStubs: func(reg *httpmock.Registry) { + stubKeywordSearch(reg, `{"total_count": 1, "incomplete_results": false, "items": [{"name": "SKILL.md", "path": "skills/terraform/SKILL.md", "repository": {"full_name": "org/repo"}}]}`) + }, + wantErr: `no skills found on page 999 for query "terraform"`, + }, + { + name: "json output with selected fields", + tty: false, + opts: func() *searchOptions { + exporter := cmdutil.NewJSONExporter() + exporter.SetFields([]string{"repo", "skillName", "stars"}) + return &searchOptions{Query: "terraform", Page: 1, Limit: defaultLimit, Exporter: exporter} + }(), + httpStubs: func(reg *httpmock.Registry) { + stubKeywordSearch(reg, `{"total_count": 1, "incomplete_results": false, "items": [{"name": "SKILL.md", "path": "skills/terraform/SKILL.md", "repository": {"full_name": "github/awesome-skills"}}]}`) + }, + wantStdout: "[{\"repo\":\"github/awesome-skills\",\"skillName\":\"terraform\",\"stars\":0}]\n", + }, + { + name: "json output empty results", + tty: false, + opts: func() *searchOptions { + exporter := cmdutil.NewJSONExporter() + exporter.SetFields([]string{"repo", "skillName"}) + return &searchOptions{Query: "nonexistent", Page: 1, Limit: defaultLimit, Exporter: exporter} + }(), + httpStubs: func(reg *httpmock.Registry) { + stubKeywordSearch(reg, emptyCodeResponse) + }, + wantStdout: "[]\n", + }, + { + name: "rate limit error returns friendly message", + tty: false, + opts: &searchOptions{Query: "terraform", Page: 1, Limit: defaultLimit}, + httpStubs: func(reg *httpmock.Registry) { + // All search/code calls return 403 with x-ratelimit-remaining: 0 + for range 3 { + reg.Register( + httpmock.REST("GET", "search/code"), + httpmock.WithHeader( + httpmock.StatusJSONResponse(403, map[string]string{"message": "API rate limit exceeded"}), + "x-ratelimit-remaining", "0", + ), + ) + } + }, + wantErr: rateLimitErrorMessage, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + reg := &httpmock.Registry{} + if tt.httpStubs != nil { + tt.httpStubs(reg) + } + tt.opts.HttpClient = func() (*http.Client, error) { + return &http.Client{Transport: reg}, nil + } + tt.opts.Config = func() (gh.Config, error) { + return config.NewBlankConfig(), nil + } + + ios, _, stdout, stderr := iostreams.Test() + ios.SetStdoutTTY(tt.tty) + ios.SetStderrTTY(tt.tty) + tt.opts.IO = ios + + defer reg.Verify(t) + err := searchRun(tt.opts) + + if tt.wantErr != "" { + require.Error(t, err) + assert.Contains(t, err.Error(), tt.wantErr) + return + } + + require.NoError(t, err) + assert.Equal(t, tt.wantStdout, stdout.String()) + assert.Equal(t, tt.wantStderr, stderr.String()) + }) + } +} + +func TestDeduplicateResults(t *testing.T) { + items := []codeSearchItem{ + {Path: "skills/terraform/SKILL.md", Repository: codeSearchRepository{FullName: "org/repo"}}, + {Path: "skills/terraform/SKILL.md", Repository: codeSearchRepository{FullName: "org/repo"}}, + {Path: "skills/docker/SKILL.md", Repository: codeSearchRepository{FullName: "org/repo"}}, + {Path: "skills/terraform/SKILL.md", Repository: codeSearchRepository{FullName: "other/repo"}}, + } + + results := deduplicateResults(items) + + assert.Equal(t, 3, len(results)) + assert.Equal(t, "org/repo", results[0].Repo) + assert.Equal(t, "org", results[0].Owner) + assert.Equal(t, "repo", results[0].RepoName) + assert.Equal(t, "terraform", results[0].SkillName) + assert.Equal(t, "docker", results[1].SkillName) + assert.Equal(t, "other/repo", results[2].Repo) + assert.Equal(t, "other", results[2].Owner) + assert.Equal(t, "terraform", results[2].SkillName) +} + +func TestExtractSkillName(t *testing.T) { + tests := []struct { + path string + want string + }{ + {"skills/terraform/SKILL.md", "terraform"}, + {"skills/author/my-skill/SKILL.md", "my-skill"}, + {"SKILL.md", ""}, + {"skills/docker/SKILL.md", "docker"}, + // Root-level convention + {"my-skill/SKILL.md", "my-skill"}, + // Plugins convention + {"plugins/openai/skills/chat/SKILL.md", "chat"}, + // Non-matching paths should be filtered out + {"random/nested/deep/SKILL.md", ""}, + {".hidden/SKILL.md", ""}, + } + + for _, tt := range tests { + t.Run(tt.path, func(t *testing.T) { + got := extractSkillName(tt.path) + assert.Equal(t, tt.want, got) + }) + } +} + +func TestFilterByRelevance(t *testing.T) { + skills := []skillResult{ + {Repo: "org/repo1", Owner: "org", RepoName: "repo1", SkillName: "terraform"}, + {Repo: "org/repo2", Owner: "org", RepoName: "repo2", SkillName: "docker"}, + {Repo: "terraform-corp/tools", Owner: "terraform-corp", RepoName: "tools", SkillName: "linter"}, + {Repo: "acme/terraform-tools", Owner: "acme", RepoName: "terraform-tools", SkillName: "validator"}, + {Repo: "x/y", Owner: "x", RepoName: "y", SkillName: "unrelated", Description: "terraform integration"}, + {Repo: "x/z", Owner: "x", RepoName: "z", SkillName: "noise"}, + } + + filtered := filterByRelevance(skills, "terraform") + + // Should keep: name match (terraform), owner match (terraform-corp), + // repo name match (terraform-tools), description match (terraform integration). + // Should drop: docker, noise. + assert.Equal(t, 4, len(filtered)) + assert.Equal(t, "terraform", filtered[0].SkillName) + assert.Equal(t, "linter", filtered[1].SkillName) + assert.Equal(t, "validator", filtered[2].SkillName) + assert.Equal(t, "unrelated", filtered[3].SkillName) +} + +func TestRankByRelevance(t *testing.T) { + skills := []skillResult{ + {Repo: "org/repo1", Owner: "org", SkillName: "devops"}, + {Repo: "org/repo2", Owner: "org", SkillName: "terraform-plan"}, + {Repo: "org/repo3", Owner: "org", SkillName: "docker", Description: "Manages terraform docker containers"}, + {Repo: "org/repo4", Owner: "org", SkillName: "terraform"}, + } + + rankByRelevance(skills, "terraform") + + // Exact name match scores highest (10 000), then partial name (1 000), + // then description match (100), then body-only (0). + assert.Equal(t, "terraform", skills[0].SkillName) + assert.Equal(t, "terraform-plan", skills[1].SkillName) + assert.Equal(t, "docker", skills[2].SkillName) + assert.Equal(t, "devops", skills[3].SkillName) +} + +func TestRankByRelevanceStarsTiebreak(t *testing.T) { + skills := []skillResult{ + {Repo: "small/repo", Owner: "small", SkillName: "terraform", Stars: 10}, + {Repo: "big/repo", Owner: "big", SkillName: "terraform", Stars: 5000}, + } + + rankByRelevance(skills, "terraform") + + // Both have exact name match; big/repo wins on stars tiebreak + assert.Equal(t, "big/repo", skills[0].Repo) + assert.Equal(t, "small/repo", skills[1].Repo) +} + +func TestFormatStars(t *testing.T) { + assert.Equal(t, "0", formatStars(0)) + assert.Equal(t, "42", formatStars(42)) + assert.Equal(t, "999", formatStars(999)) + assert.Equal(t, "1.0k", formatStars(1000)) + assert.Equal(t, "1.7k", formatStars(1700)) + assert.Equal(t, "12.5k", formatStars(12500)) +} + +func splitOnSpaces(s string) []string { + var parts []string + current := "" + for _, c := range s { + if c == ' ' { + if current != "" { + parts = append(parts, current) + current = "" + } + } else { + current += string(c) + } + } + if current != "" { + parts = append(parts, current) + } + return parts +} + +type discardWriter struct{} + +func (d *discardWriter) Write(p []byte) (n int, err error) { + return len(p), nil +} diff --git a/pkg/cmd/skills/skills.go b/pkg/cmd/skills/skills.go index 61afc12a4..8a1367314 100644 --- a/pkg/cmd/skills/skills.go +++ b/pkg/cmd/skills/skills.go @@ -2,6 +2,10 @@ package skills import ( "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" + "github.com/cli/cli/v2/pkg/cmd/skills/search" + "github.com/cli/cli/v2/pkg/cmd/skills/update" "github.com/cli/cli/v2/pkg/cmdutil" "github.com/spf13/cobra" ) @@ -16,6 +20,10 @@ func NewCmdSkills(f *cmdutil.Factory) *cobra.Command { } cmd.AddCommand(install.NewCmdInstall(f, nil)) + cmd.AddCommand(preview.NewCmdPreview(f, nil)) + cmd.AddCommand(publish.NewCmdPublish(f, nil)) + cmd.AddCommand(search.NewCmdSearch(f, nil)) + cmd.AddCommand(update.NewCmdUpdate(f, nil)) return cmd } diff --git a/pkg/cmd/skills/update/update.go b/pkg/cmd/skills/update/update.go new file mode 100644 index 000000000..42995a315 --- /dev/null +++ b/pkg/cmd/skills/update/update.go @@ -0,0 +1,560 @@ +package update + +import ( + "context" + "fmt" + "net/http" + "os" + "path/filepath" + "strings" + + "github.com/MakeNowJust/heredoc" + "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/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/installer" + "github.com/cli/cli/v2/internal/skills/registry" + "github.com/cli/cli/v2/pkg/cmdutil" + "github.com/cli/cli/v2/pkg/iostreams" + "github.com/spf13/cobra" +) + +// updateOptions holds all dependencies and user-provided flags for the update command. +type updateOptions struct { + IO *iostreams.IOStreams + HttpClient func() (*http.Client, error) + Config func() (gh.Config, error) + Prompter prompter.Prompter + GitClient *git.Client + + // Arguments + Skills []string // optional: specific skills to update + + // Flags + All bool // --all flag (update without prompting) + Force bool // --force flag (re-download even if SHAs match) + DryRun bool // --dry-run flag (report only, no changes) + Dir string // --dir flag (scan a custom directory) +} + +// 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 +} + +// pendingUpdate describes a single skill that has an available update. +type pendingUpdate struct { + local installedSkill + newSHA string // new tree SHA from remote + resolved *discovery.ResolvedRef + skill discovery.Skill +} + +// NewCmdUpdate creates the "skills update" command. +func NewCmdUpdate(f *cmdutil.Factory, runF func(*updateOptions) error) *cobra.Command { + opts := &updateOptions{ + IO: f.IOStreams, + Prompter: f.Prompter, + Config: f.Config, + GitClient: f.GitClient, + HttpClient: f.HttpClient, + } + + cmd := &cobra.Command{ + Use: "update [...]", + Short: "Update installed skills to their latest versions", + Long: heredoc.Doc(` + Checks installed skills for available updates by comparing the local + tree SHA (from SKILL.md frontmatter) against the remote repository. + + Scans all known agent host directories (Copilot, Claude, Cursor, Codex, + Gemini, Antigravity) in both project and user scope automatically. + + Without arguments, checks all installed skills. With skill names, + checks only those specific skills. + + Pinned skills (installed with --pin) are skipped with a notice. + Use "gh skills install --pin " to change the pinned version. + + Skills without GitHub metadata (e.g. installed manually or by another + tool) are prompted for their source repository in interactive mode. + The update re-downloads the skill with metadata injected, so future + updates work automatically. + + With --force, re-downloads skills even when the remote version matches + the local tree SHA. This overwrites locally modified skill files with + their original content, but does not remove extra files added locally. + + In interactive mode, shows which skills have updates and asks for + confirmation before proceeding. With --all, updates without prompting. + With --dry-run, reports available updates without modifying any files. + `), + Example: heredoc.Doc(` + # Check and update all skills interactively + $ gh skills update + + # Update specific skills + $ gh skills update mcp-cli git-commit + + # Update all without prompting + $ gh skills update --all + + # Re-download all skills (restore locally modified files) + $ gh skills update --force --all + + # Check for updates without applying (read-only) + $ gh skills update --dry-run + `), + RunE: func(cmd *cobra.Command, args []string) error { + opts.Skills = args + if runF != nil { + return runF(opts) + } + return updateRun(opts) + }, + } + + cmd.Flags().BoolVar(&opts.All, "all", false, "Update all skills without prompting") + cmd.Flags().BoolVar(&opts.Force, "force", false, "Re-download even if already up to date") + cmd.Flags().BoolVar(&opts.DryRun, "dry-run", false, "Report available updates without modifying files") + cmd.Flags().StringVar(&opts.Dir, "dir", "", "Scan a custom directory for installed skills") + + return cmd +} + +func updateRun(opts *updateOptions) error { + cs := opts.IO.ColorScheme() + canPrompt := opts.IO.CanPrompt() + + httpClient, err := opts.HttpClient() + if err != nil { + return err + } + apiClient := api.NewClientFromHTTP(httpClient) + + cfg, err := opts.Config() + if err != nil { + return err + } + hostname, _ := cfg.Authentication().DefaultHost() + + gitRoot := resolveGitRoot(opts.GitClient) + homeDir := resolveHomeDir() + + // Scan for installed skills + var installed []installedSkill + if opts.Dir != "" { + skills, scanErr := scanInstalledSkills(opts.Dir, nil, "") + if scanErr != nil { + return fmt.Errorf("could not scan directory: %w", scanErr) + } + installed = skills + } else { + installed = scanAllHosts(gitRoot, homeDir) + } + + if len(installed) == 0 { + fmt.Fprintf(opts.IO.ErrOut, "No installed skills found.\n") + return nil + } + + // Filter to requested skills if specified + if len(opts.Skills) > 0 { + requested := make(map[string]bool, len(opts.Skills)) + for _, name := range opts.Skills { + requested[name] = true + } + var filtered []installedSkill + for _, s := range installed { + if requested[s.name] { + filtered = append(filtered, s) + } + } + if len(filtered) == 0 { + return fmt.Errorf("none of the specified skills are installed") + } + installed = filtered + } + + // Prompt for metadata on skills missing it (before starting progress indicator) + var noMeta []string + // Track skills where the user provided a source repo interactively. + // Keyed by directory path to avoid collisions when the same skill name + // is installed across multiple hosts or scopes. + type promptedEntry struct { + name string + source string // "owner/repo" + } + prompted := make(map[string]promptedEntry) // dir → entry + for i := range installed { + s := &installed[i] + if s.owner != "" && s.repo != "" { + continue + } + if !canPrompt { + noMeta = append(noMeta, s.name) + continue + } + fmt.Fprintf(opts.IO.ErrOut, "%s %s has no GitHub metadata\n", cs.WarningIcon(), s.name) + owner, repo, reason, ok, promptErr := promptForSkillOrigin(opts.Prompter, s.name) + if promptErr != nil { + return promptErr + } + if !ok { + if reason != "" { + fmt.Fprintf(opts.IO.ErrOut, " %s %s\n", cs.WarningIcon(), reason) + } + fmt.Fprintf(opts.IO.ErrOut, " Skipping %s\n", s.name) + continue + } + s.owner = owner + s.repo = repo + prompted[s.dir] = promptedEntry{name: s.name, source: owner + "/" + repo} + } + + opts.IO.StartProgressIndicatorWithLabel(fmt.Sprintf("Checking %d installed skill(s) for updates", len(installed))) + + var updates []pendingUpdate + var pinned []installedSkill + + type repoKey struct{ owner, repo string } + repoSkills := make(map[repoKey][]discovery.Skill) + repoRefs := make(map[repoKey]*discovery.ResolvedRef) + repoErrors := make(map[repoKey]bool) + + for _, s := range installed { + if s.owner == "" || s.repo == "" { + continue + } + if s.pinned != "" { + pinned = append(pinned, s) + continue + } + + key := repoKey{s.owner, s.repo} + + if repoErrors[key] { + continue + } + + // Resolve ref and discover skills once per repo + if _, ok := repoRefs[key]; !ok { + resolved, resolveErr := discovery.ResolveRef(apiClient, hostname, s.owner, s.repo, "") + if resolveErr != nil { + repoErrors[key] = true + opts.IO.StopProgressIndicator() + fmt.Fprintf(opts.IO.ErrOut, "%s Skipping %s: could not resolve %s/%s: %v\n", cs.WarningIcon(), s.name, s.owner, s.repo, resolveErr) + opts.IO.StartProgressIndicatorWithLabel(fmt.Sprintf("Checking %d installed skill(s) for updates", len(installed))) + continue + } + repoRefs[key] = resolved + + skills, discoverErr := discovery.DiscoverSkills(apiClient, hostname, s.owner, s.repo, resolved.SHA) + if discoverErr != nil { + repoErrors[key] = true + opts.IO.StopProgressIndicator() + fmt.Fprintf(opts.IO.ErrOut, "%s Skipping %s: %v\n", cs.WarningIcon(), s.name, discoverErr) + opts.IO.StartProgressIndicatorWithLabel(fmt.Sprintf("Checking %d installed skill(s) for updates", len(installed))) + continue + } + repoSkills[key] = skills + } + + resolved := repoRefs[key] + for _, remote := range repoSkills[key] { + matched := false + if s.sourcePath != "" { + matched = remote.Path == s.sourcePath + } else { + matched = remote.InstallName() == s.name + } + if matched && (remote.TreeSHA != s.treeSHA || opts.Force) { + updates = append(updates, pendingUpdate{ + local: s, + newSHA: remote.TreeSHA, + resolved: resolved, + skill: remote, + }) + break + } + } + } + + opts.IO.StopProgressIndicator() + + // 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]} + skills, resolved := repoSkills[key] + if !resolved { + continue + } + found := false + for _, remote := range skills { + if remote.InstallName() == entry.name || remote.Name == entry.name { + found = true + break + } + } + if !found { + fmt.Fprintf(opts.IO.ErrOut, "%s Skill %s not found in %s\n", cs.WarningIcon(), entry.name, entry.source) + } + } + + for _, s := range pinned { + fmt.Fprintf(opts.IO.ErrOut, "%s %s is pinned to %s (skipped)\n", cs.Gray("⊘"), s.name, s.pinned) + } + for _, name := range noMeta { + fmt.Fprintf(opts.IO.ErrOut, "%s %s has no GitHub metadata — reinstall to enable updates\n", cs.WarningIcon(), name) + } + + if len(updates) == 0 { + if opts.Force && opts.DryRun { + fmt.Fprintf(opts.IO.ErrOut, "All skills are up to date. Use --force without --dry-run to re-download anyway.\n") + } else { + fmt.Fprintf(opts.IO.ErrOut, "All skills are up to date.\n") + } + return nil + } + + fmt.Fprintf(opts.IO.ErrOut, "\n%d update(s) available:\n", len(updates)) + for _, u := range updates { + if u.local.treeSHA == u.newSHA { + fmt.Fprintf(opts.IO.Out, " %s %s (%s/%s) %s (reinstall) [%s]\n", + cs.Cyan("•"), u.local.name, u.local.owner, u.local.repo, + git.ShortSHA(u.newSHA), u.resolved.Ref) + } else { + fmt.Fprintf(opts.IO.Out, " %s %s (%s/%s) %s → %s [%s]\n", + cs.Cyan("•"), u.local.name, u.local.owner, u.local.repo, + cs.Gray(git.ShortSHA(u.local.treeSHA)), git.ShortSHA(u.newSHA), + u.resolved.Ref) + } + } + fmt.Fprintln(opts.IO.ErrOut) + + if opts.DryRun { + return nil + } + + if !opts.All { + if !canPrompt { + return fmt.Errorf("updates available; re-run with --all to apply, or run interactively to confirm") + } + confirmed, confirmErr := opts.Prompter.Confirm(fmt.Sprintf("Update %d skill(s)?", len(updates)), true) + if confirmErr != nil { + return confirmErr + } + if !confirmed { + fmt.Fprintf(opts.IO.ErrOut, "Update cancelled.\n") + return nil + } + } + + var failed bool + for _, u := range updates { + installOpts := &installer.Options{ + Host: hostname, + Owner: u.local.owner, + Repo: u.local.repo, + Ref: u.resolved.Ref, + SHA: u.resolved.SHA, + Skills: []discovery.Skill{u.skill}, + AgentHost: u.local.host, + Scope: u.local.scope, + GitRoot: gitRoot, + HomeDir: homeDir, + Client: apiClient, + } + // When updating skills from a custom --dir, host is nil. + // Use the skill's install root as the target. For namespaced + // skills (name contains "/"), the dir is two levels below the + // root instead of one. + if u.local.host == nil { + base := filepath.Dir(u.local.dir) + if strings.Contains(u.local.name, "/") { + base = filepath.Dir(base) + } + installOpts.Dir = base + } + _, installErr := installer.Install(installOpts) + if installErr != nil { + fmt.Fprintf(opts.IO.ErrOut, "%s Failed to update %s: %v\n", cs.FailureIcon(), u.local.name, installErr) + failed = true + continue + } + if opts.IO.IsStdoutTTY() { + fmt.Fprintf(opts.IO.Out, "%s Updated %s\n", cs.SuccessIcon(), u.local.name) + } else { + fmt.Fprintf(opts.IO.Out, "Updated %s\n", u.local.name) + } + } + + if failed { + return cmdutil.SilentError + } + + return nil +} + +// scanAllHosts walks every known host directory (project + user scope) and +// collects installed skills. Skills are deduplicated by directory path. +func scanAllHosts(gitRoot, homeDir string) []installedSkill { + seen := make(map[string]bool) + var all []installedSkill + + for i := range registry.Agents { + host := ®istry.Agents[i] + for _, scope := range []registry.Scope{registry.ScopeProject, registry.ScopeUser} { + dir, err := host.InstallDir(scope, gitRoot, homeDir) + if err != nil { + continue + } + skills, err := scanInstalledSkills(dir, host, scope) + if err != nil { + continue + } + for _, s := range skills { + if seen[s.dir] { + continue + } + seen[s.dir] = true + all = append(all, s) + } + } + } + + return all +} + +// scanInstalledSkills reads all SKILL.md files in a skills directory and +// extracts GitHub metadata from their frontmatter. It handles both flat +// layouts ({dir}/{name}/SKILL.md) and namespaced layouts +// ({dir}/{namespace}/{name}/SKILL.md). +func scanInstalledSkills(skillsDir string, host *registry.AgentHost, scope registry.Scope) ([]installedSkill, error) { + entries, err := os.ReadDir(skillsDir) + if os.IsNotExist(err) { + return nil, nil + } + if err != nil { + return nil, fmt.Errorf("could not read skills directory: %w", err) + } + + var skills []installedSkill + for _, e := range entries { + if !e.IsDir() { + continue + } + + // Flat layout: {dir}/{name}/SKILL.md + skillFile := filepath.Join(skillsDir, e.Name(), "SKILL.md") + if data, readErr := os.ReadFile(skillFile); readErr == nil { + if s, ok := parseInstalledSkill(data, e.Name(), filepath.Join(skillsDir, e.Name()), host, scope); ok { + skills = append(skills, s) + continue + } + } + + // Namespaced layout: {dir}/{namespace}/{name}/SKILL.md + subEntries, subErr := os.ReadDir(filepath.Join(skillsDir, e.Name())) + if subErr != nil { + continue + } + for _, sub := range subEntries { + if !sub.IsDir() { + continue + } + subSkillFile := filepath.Join(skillsDir, e.Name(), sub.Name(), "SKILL.md") + if data, readErr := os.ReadFile(subSkillFile); readErr == nil { + installName := e.Name() + "/" + sub.Name() + if s, ok := parseInstalledSkill(data, installName, filepath.Join(skillsDir, e.Name(), sub.Name()), host, scope); ok { + skills = append(skills, s) + } + } + } + } + + return skills, nil +} + +// parseInstalledSkill parses a SKILL.md file and returns an installedSkill. +func parseInstalledSkill(data []byte, name, dir string, host *registry.AgentHost, scope registry.Scope) (installedSkill, bool) { + result, err := frontmatter.Parse(string(data)) + if err != nil { + return installedSkill{}, false + } + + s := installedSkill{ + name: name, + dir: dir, + host: host, + scope: scope, + } + + if result.Metadata.Meta != nil { + s.owner, _ = result.Metadata.Meta["github-owner"].(string) + s.repo, _ = result.Metadata.Meta["github-repo"].(string) + 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) + } + + return s, true +} + +// promptForSkillOrigin asks the user for the source repository of a skill +// that has no GitHub metadata. +func promptForSkillOrigin(p prompter.Prompter, skillName string) (owner, repo, reason string, ok bool, err error) { + input, err := p.Input( + fmt.Sprintf("Repository for %s (owner/repo):", skillName), "") + if err != nil { + return "", "", "", false, err + } + input = strings.TrimSpace(input) + if input == "" { + return "", "", "", false, nil + } + r, err := ghrepo.FromFullName(input) + if err != nil { + //nolint:nilerr // intentionally converting parse error into a user-facing validation message + return "", "", fmt.Sprintf("invalid repository %q: expected owner/repo", input), false, nil + } + return r.RepoOwner(), r.RepoName(), "", true, nil +} + +func resolveGitRoot(gc *git.Client) string { + if gc == nil { + if cwd, err := os.Getwd(); err == nil { + return cwd + } + return "" + } + root, err := gc.ToplevelDir(context.Background()) + if err != nil { + if cwd, err := os.Getwd(); err == nil { + return cwd + } + return "" + } + return root +} + +func resolveHomeDir() string { + home, err := os.UserHomeDir() + if err != nil { + return "" + } + return home +} diff --git a/pkg/cmd/skills/update/update_test.go b/pkg/cmd/skills/update/update_test.go new file mode 100644 index 000000000..735536b0d --- /dev/null +++ b/pkg/cmd/skills/update/update_test.go @@ -0,0 +1,391 @@ +package update + +import ( + "fmt" + "net/http" + "os" + "path/filepath" + "testing" + + "github.com/cli/cli/v2/git" + "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/pkg/cmdutil" + "github.com/cli/cli/v2/pkg/httpmock" + "github.com/cli/cli/v2/pkg/iostreams" + "github.com/google/shlex" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +func TestNewCmdUpdate_Help(t *testing.T) { + ios, _, _, _ := iostreams.Test() + f := &cmdutil.Factory{ + IOStreams: ios, + Prompter: &prompter.PrompterMock{}, + GitClient: &git.Client{}, + } + + cmd := NewCmdUpdate(f, func(opts *updateOptions) error { + return nil + }) + + assert.Equal(t, "update [...]", cmd.Use) + assert.NotEmpty(t, cmd.Short) + assert.NotEmpty(t, cmd.Long) + assert.NotEmpty(t, cmd.Example) +} + +func TestNewCmdUpdate_Flags(t *testing.T) { + ios, _, _, _ := iostreams.Test() + f := &cmdutil.Factory{IOStreams: ios, Prompter: &prompter.PrompterMock{}, GitClient: &git.Client{}} + cmd := NewCmdUpdate(f, func(_ *updateOptions) error { return nil }) + + flags := []string{"all", "force", "dry-run", "dir"} + for _, name := range flags { + assert.NotNil(t, cmd.Flags().Lookup(name), "missing flag: --%s", name) + } +} + +func TestNewCmdUpdate_ArgsPassedToOptions(t *testing.T) { + ios, _, _, _ := iostreams.Test() + f := &cmdutil.Factory{IOStreams: ios, Prompter: &prompter.PrompterMock{}, GitClient: &git.Client{}} + + var gotOpts *updateOptions + cmd := NewCmdUpdate(f, func(opts *updateOptions) error { + gotOpts = opts + return nil + }) + + args, _ := shlex.Split("mcp-cli git-commit --all --force") + cmd.SetArgs(args) + cmd.SetOut(os.Stdout) + cmd.SetErr(os.Stderr) + err := cmd.Execute() + require.NoError(t, err) + assert.Equal(t, []string{"mcp-cli", "git-commit"}, gotOpts.Skills) + assert.True(t, gotOpts.All) + assert.True(t, gotOpts.Force) +} + +func TestScanInstalledSkills(t *testing.T) { + dir := t.TempDir() + + skillDir := filepath.Join(dir, "git-commit") + require.NoError(t, os.MkdirAll(skillDir, 0o755)) + content := "---\nname: git-commit\ndescription: Git commit helper\nmetadata:\n github-owner: github\n github-repo: awesome-copilot\n github-tree-sha: abc123\n github-path: skills/git-commit\n---\nBody content\n" + require.NoError(t, os.WriteFile(filepath.Join(skillDir, "SKILL.md"), []byte(content), 0o644)) + + noMetaDir := filepath.Join(dir, "unknown-skill") + require.NoError(t, os.MkdirAll(noMetaDir, 0o755)) + require.NoError(t, os.WriteFile(filepath.Join(noMetaDir, "SKILL.md"), []byte("---\nname: unknown-skill\n---\nNo metadata here\n"), 0o644)) + + pinnedDir := filepath.Join(dir, "pinned-skill") + require.NoError(t, os.MkdirAll(pinnedDir, 0o755)) + require.NoError(t, os.WriteFile(filepath.Join(pinnedDir, "SKILL.md"), []byte("---\nname: pinned-skill\nmetadata:\n github-owner: octo\n github-repo: skills\n github-tree-sha: def456\n github-pinned: v1.0.0\n---\nPinned content\n"), 0o644)) + + skills, err := scanInstalledSkills(dir, nil, "") + require.NoError(t, err) + assert.Len(t, skills, 3) + + byName := make(map[string]installedSkill) + for _, s := range skills { + byName[s.name] = s + } + + gc := byName["git-commit"] + assert.Equal(t, "github", gc.owner) + assert.Equal(t, "awesome-copilot", gc.repo) + assert.Equal(t, "abc123", gc.treeSHA) + assert.Equal(t, "skills/git-commit", gc.sourcePath) + assert.Empty(t, gc.pinned) + + us := byName["unknown-skill"] + assert.Empty(t, us.owner) + assert.Empty(t, us.repo) + + ps := byName["pinned-skill"] + assert.Equal(t, "v1.0.0", ps.pinned) +} + +func TestScanInstalledSkills_NonExistentDir(t *testing.T) { + skills, err := scanInstalledSkills("/nonexistent/path", nil, "") + require.NoError(t, err) + assert.Nil(t, skills) +} + +func TestScanInstalledSkills_CorruptedYAML(t *testing.T) { + dir := t.TempDir() + skillDir := filepath.Join(dir, "corrupt") + require.NoError(t, os.MkdirAll(skillDir, 0o755)) + require.NoError(t, os.WriteFile(filepath.Join(skillDir, "SKILL.md"), []byte("---\nnot: valid: yaml: [broken\n---\nbody\n"), 0o644)) + + skills, err := scanInstalledSkills(dir, nil, "") + require.NoError(t, err) + assert.Len(t, skills, 0) +} + +func TestPromptForSkillOrigin_Valid(t *testing.T) { + pm := &prompter.PrompterMock{ + InputFunc: func(prompt string, defaultValue string) (string, error) { + return "github/awesome-copilot", nil + }, + } + owner, repo, _, ok, err := promptForSkillOrigin(pm, "test-skill") + require.NoError(t, err) + assert.True(t, ok) + assert.Equal(t, "github", owner) + assert.Equal(t, "awesome-copilot", repo) +} + +func TestPromptForSkillOrigin_Empty(t *testing.T) { + pm := &prompter.PrompterMock{ + InputFunc: func(prompt string, defaultValue string) (string, error) { + return "", nil + }, + } + _, _, _, ok, err := promptForSkillOrigin(pm, "test-skill") + require.NoError(t, err) + assert.False(t, ok) +} + +func TestPromptForSkillOrigin_Invalid(t *testing.T) { + pm := &prompter.PrompterMock{ + InputFunc: func(prompt string, defaultValue string) (string, error) { + return "just-a-name", nil + }, + } + _, _, reason, ok, err := promptForSkillOrigin(pm, "test-skill") + require.NoError(t, err) + assert.False(t, ok) + assert.Contains(t, reason, "invalid repository") +} + +func TestUpdateRun_NoInstalledSkills(t *testing.T) { + ios, _, _, stderr := iostreams.Test() + ios.SetStdoutTTY(false) + + dir := t.TempDir() + + reg := &httpmock.Registry{} + opts := &updateOptions{ + IO: ios, + Config: func() (gh.Config, error) { return config.NewBlankConfig(), nil }, + HttpClient: func() (*http.Client, error) { + return &http.Client{Transport: reg}, nil + }, + GitClient: &git.Client{RepoDir: dir}, + Dir: dir, + } + + defer reg.Verify(t) + err := updateRun(opts) + require.NoError(t, err) + assert.Contains(t, stderr.String(), "No installed skills found.") +} + +func TestUpdateRun_SpecificSkillNotInstalled(t *testing.T) { + ios, _, _, _ := iostreams.Test() + ios.SetStdoutTTY(false) + + dir := t.TempDir() + skillDir := filepath.Join(dir, "existing-skill") + require.NoError(t, os.MkdirAll(skillDir, 0o755)) + require.NoError(t, os.WriteFile(filepath.Join(skillDir, "SKILL.md"), []byte("---\nname: existing-skill\nmetadata:\n github-owner: owner\n github-repo: repo\n github-tree-sha: abc\n---\n"), 0o644)) + + reg := &httpmock.Registry{} + opts := &updateOptions{ + IO: ios, + Config: func() (gh.Config, error) { return config.NewBlankConfig(), nil }, + HttpClient: func() (*http.Client, error) { + return &http.Client{Transport: reg}, nil + }, + GitClient: &git.Client{RepoDir: dir}, + Dir: dir, + Skills: []string{"nonexistent"}, + } + + defer reg.Verify(t) + err := updateRun(opts) + assert.EqualError(t, err, "none of the specified skills are installed") +} + +func TestUpdateRun_PinnedSkillsSkipped(t *testing.T) { + ios, _, _, stderr := iostreams.Test() + ios.SetStdoutTTY(true) + ios.SetStderrTTY(true) + + dir := t.TempDir() + skillDir := filepath.Join(dir, "pinned-skill") + require.NoError(t, os.MkdirAll(skillDir, 0o755)) + require.NoError(t, os.WriteFile(filepath.Join(skillDir, "SKILL.md"), []byte("---\nname: pinned-skill\nmetadata:\n github-owner: owner\n github-repo: repo\n github-tree-sha: abc123\n github-pinned: v1.0.0\n---\n"), 0o644)) + + reg := &httpmock.Registry{} + opts := &updateOptions{ + IO: ios, + Config: func() (gh.Config, error) { return config.NewBlankConfig(), nil }, + HttpClient: func() (*http.Client, error) { + return &http.Client{Transport: reg}, nil + }, + Prompter: &prompter.PrompterMock{}, + GitClient: &git.Client{RepoDir: dir}, + Dir: dir, + } + + defer reg.Verify(t) + err := updateRun(opts) + require.NoError(t, err) + assert.Contains(t, stderr.String(), "pinned-skill is pinned to v1.0.0 (skipped)") + assert.Contains(t, stderr.String(), "All skills are up to date.") +} + +func TestUpdateRun_NoMetaSkipsNonInteractive(t *testing.T) { + ios, _, _, stderr := iostreams.Test() + ios.SetStdoutTTY(false) + ios.SetStdinTTY(false) + + dir := t.TempDir() + skillDir := filepath.Join(dir, "manual-skill") + require.NoError(t, os.MkdirAll(skillDir, 0o755)) + require.NoError(t, os.WriteFile(filepath.Join(skillDir, "SKILL.md"), []byte("---\nname: manual-skill\n---\nNo metadata\n"), 0o644)) + + reg := &httpmock.Registry{} + opts := &updateOptions{ + IO: ios, + Config: func() (gh.Config, error) { return config.NewBlankConfig(), nil }, + HttpClient: func() (*http.Client, error) { + return &http.Client{Transport: reg}, nil + }, + GitClient: &git.Client{RepoDir: dir}, + Dir: dir, + } + + defer reg.Verify(t) + err := updateRun(opts) + require.NoError(t, err) + assert.Contains(t, stderr.String(), "manual-skill has no GitHub metadata") +} + +func TestUpdateRun_AllUpToDate(t *testing.T) { + ios, _, _, stderr := iostreams.Test() + ios.SetStdoutTTY(false) + + dir := t.TempDir() + skillDir := filepath.Join(dir, "my-skill") + require.NoError(t, os.MkdirAll(skillDir, 0o755)) + require.NoError(t, os.WriteFile(filepath.Join(skillDir, "SKILL.md"), []byte("---\nname: my-skill\nmetadata:\n github-owner: octo\n github-repo: skills\n github-tree-sha: abc123def456\n github-path: skills/my-skill\n---\n"), 0o644)) + + reg := &httpmock.Registry{} + reg.Register( + httpmock.REST("GET", "repos/octo/skills/releases/latest"), + httpmock.StringResponse(`{"tag_name": "v1.0.0"}`), + ) + reg.Register( + httpmock.REST("GET", "repos/octo/skills/git/ref/tags/v1.0.0"), + httpmock.StringResponse(`{"object": {"sha": "commitsha123", "type": "commit"}}`), + ) + reg.Register( + httpmock.REST("GET", fmt.Sprintf("repos/octo/skills/git/trees/commitsha123")), + httpmock.StringResponse(`{"sha": "commitsha123", "tree": [{"path": "skills/my-skill/SKILL.md", "type": "blob", "sha": "blobsha1"}, {"path": "skills/my-skill", "type": "tree", "sha": "abc123def456"}, {"path": "skills", "type": "tree", "sha": "treeshaX"}], "truncated": false}`), + ) + + opts := &updateOptions{ + IO: ios, + Config: func() (gh.Config, error) { return config.NewBlankConfig(), nil }, + HttpClient: func() (*http.Client, error) { + return &http.Client{Transport: reg}, nil + }, + GitClient: &git.Client{RepoDir: dir}, + Dir: dir, + } + + defer reg.Verify(t) + err := updateRun(opts) + require.NoError(t, err) + assert.Contains(t, stderr.String(), "All skills are up to date.") +} + +func TestUpdateRun_DryRun(t *testing.T) { + ios, _, stdout, stderr := iostreams.Test() + ios.SetStdoutTTY(true) + ios.SetStderrTTY(true) + + dir := t.TempDir() + skillDir := filepath.Join(dir, "my-skill") + require.NoError(t, os.MkdirAll(skillDir, 0o755)) + require.NoError(t, os.WriteFile(filepath.Join(skillDir, "SKILL.md"), []byte("---\nname: my-skill\nmetadata:\n github-owner: octo\n github-repo: skills\n github-tree-sha: oldsha123\n github-path: skills/my-skill\n---\n"), 0o644)) + + reg := &httpmock.Registry{} + reg.Register( + httpmock.REST("GET", "repos/octo/skills/releases/latest"), + httpmock.StringResponse(`{"tag_name": "v2.0.0"}`), + ) + reg.Register( + httpmock.REST("GET", "repos/octo/skills/git/ref/tags/v2.0.0"), + httpmock.StringResponse(`{"object": {"sha": "newcommit456", "type": "commit"}}`), + ) + reg.Register( + httpmock.REST("GET", "repos/octo/skills/git/trees/newcommit456"), + httpmock.StringResponse(`{"sha": "newcommit456", "tree": [{"path": "skills/my-skill/SKILL.md", "type": "blob", "sha": "blobsha2"}, {"path": "skills/my-skill", "type": "tree", "sha": "newsha456"}, {"path": "skills", "type": "tree", "sha": "treeshaY"}], "truncated": false}`), + ) + + opts := &updateOptions{ + IO: ios, + Config: func() (gh.Config, error) { return config.NewBlankConfig(), nil }, + HttpClient: func() (*http.Client, error) { + return &http.Client{Transport: reg}, nil + }, + Prompter: &prompter.PrompterMock{}, + GitClient: &git.Client{RepoDir: dir}, + Dir: dir, + DryRun: true, + } + + defer reg.Verify(t) + err := updateRun(opts) + require.NoError(t, err) + assert.Contains(t, stderr.String(), "1 update(s) available:") + assert.Contains(t, stdout.String(), "my-skill") + assert.Contains(t, stdout.String(), "octo/skills") +} + +func TestUpdateRun_NonInteractiveNoAll(t *testing.T) { + ios, _, _, _ := iostreams.Test() + ios.SetStdoutTTY(false) + ios.SetStdinTTY(false) + + dir := t.TempDir() + skillDir := filepath.Join(dir, "my-skill") + require.NoError(t, os.MkdirAll(skillDir, 0o755)) + require.NoError(t, os.WriteFile(filepath.Join(skillDir, "SKILL.md"), []byte("---\nname: my-skill\nmetadata:\n github-owner: octo\n github-repo: skills\n github-tree-sha: oldsha123\n github-path: skills/my-skill\n---\n"), 0o644)) + + reg := &httpmock.Registry{} + reg.Register( + httpmock.REST("GET", "repos/octo/skills/releases/latest"), + httpmock.StringResponse(`{"tag_name": "v2.0.0"}`), + ) + reg.Register( + httpmock.REST("GET", "repos/octo/skills/git/ref/tags/v2.0.0"), + httpmock.StringResponse(`{"object": {"sha": "newcommit456", "type": "commit"}}`), + ) + reg.Register( + httpmock.REST("GET", "repos/octo/skills/git/trees/newcommit456"), + httpmock.StringResponse(`{"sha": "newcommit456", "tree": [{"path": "skills/my-skill/SKILL.md", "type": "blob", "sha": "blobsha2"}, {"path": "skills/my-skill", "type": "tree", "sha": "newsha456"}, {"path": "skills", "type": "tree", "sha": "treeshaY"}], "truncated": false}`), + ) + + opts := &updateOptions{ + IO: ios, + Config: func() (gh.Config, error) { return config.NewBlankConfig(), nil }, + HttpClient: func() (*http.Client, error) { + return &http.Client{Transport: reg}, nil + }, + GitClient: &git.Client{RepoDir: dir}, + Dir: dir, + } + + defer reg.Verify(t) + err := updateRun(opts) + assert.EqualError(t, err, "updates available; re-run with --all to apply, or run interactively to confirm") +}