diff --git a/.gitignore b/.gitignore index b82a00c72..ffcbbb6c5 100644 --- a/.gitignore +++ b/.gitignore @@ -38,3 +38,4 @@ *~ vendor/ +gh diff --git a/acceptance/testdata/skills/skills-install-all.txtar b/acceptance/testdata/skills/skills-install-all.txtar deleted file mode 100644 index 6efd7747e..000000000 --- a/acceptance/testdata/skills/skills-install-all.txtar +++ /dev/null @@ -1,5 +0,0 @@ -# Install all skills from a repo with mixed conventions (skills/ + plugins/) -# This previously failed with "conflicting names" — now uses namespaced dirs -exec gh skills install github/awesome-copilot --all --scope user --force --agent github-copilot -stdout 'Installed' -! stderr 'conflicting names' diff --git a/acceptance/testdata/skills/skills-install-force.txtar b/acceptance/testdata/skills/skills-install-force.txtar index 5623fce84..e6bd520b9 100644 --- a/acceptance/testdata/skills/skills-install-force.txtar +++ b/acceptance/testdata/skills/skills-install-force.txtar @@ -1,11 +1,11 @@ # Install with --force should overwrite an existing skill without error -exec gh skills install github/awesome-copilot git-commit --force --dir $WORK/force-test +exec gh skill install github/awesome-copilot git-commit --force --dir $WORK/force-test stdout 'Installed git-commit' # Install again with --force — should succeed (overwrite) -exec gh skills install github/awesome-copilot git-commit --force --dir $WORK/force-test +exec gh skill install github/awesome-copilot git-commit --force --dir $WORK/force-test stdout 'Installed git-commit' # Without --force, non-interactive should fail when skill exists -! exec gh skills install github/awesome-copilot git-commit --dir $WORK/force-test +! exec gh skill install github/awesome-copilot git-commit --dir $WORK/force-test stderr 'already installed' diff --git a/acceptance/testdata/skills/skills-install-from-local.txtar b/acceptance/testdata/skills/skills-install-from-local.txtar new file mode 100644 index 000000000..0b003fd3e --- /dev/null +++ b/acceptance/testdata/skills/skills-install-from-local.txtar @@ -0,0 +1,15 @@ +# Install from a local directory using --from-local +exec gh skill install --from-local $WORK/local-repo git-commit --dir $WORK/output --force +stdout 'Installed git-commit' + +# Verify the skill was copied +exists $WORK/output/git-commit/SKILL.md +grep 'local-path' $WORK/output/git-commit/SKILL.md + +-- local-repo/skills/git-commit/SKILL.md -- +--- +name: git-commit +description: Write good git commits +--- +# Git Commit +Body content. diff --git a/acceptance/testdata/skills/skills-install-invalid-agent.txtar b/acceptance/testdata/skills/skills-install-invalid-agent.txtar index 23883524f..7e85a9fae 100644 --- a/acceptance/testdata/skills/skills-install-invalid-agent.txtar +++ b/acceptance/testdata/skills/skills-install-invalid-agent.txtar @@ -1,4 +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' +! exec gh skill install github/awesome-copilot git-commit --agent bogus-agent --force +stderr 'invalid argument' stderr 'github-copilot' diff --git a/acceptance/testdata/skills/skills-install-invalid-repo.txtar b/acceptance/testdata/skills/skills-install-invalid-repo.txtar index 26ecbc718..2b59582e1 100644 --- a/acceptance/testdata/skills/skills-install-invalid-repo.txtar +++ b/acceptance/testdata/skills/skills-install-invalid-repo.txtar @@ -1,3 +1,3 @@ # Nonexistent repo should error -! exec gh skills install nonexistent-owner-xyz/nonexistent-repo-abc --force --dir $WORK/tmp +! exec gh skill 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 index c5cf19e56..c4fe085e4 100644 --- a/acceptance/testdata/skills/skills-install-nested-files.txtar +++ b/acceptance/testdata/skills/skills-install-nested-files.txtar @@ -1,3 +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 +exec gh skill 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 index 23f72cee8..44187c4ff 100644 --- a/acceptance/testdata/skills/skills-install-nonexistent-skill.txtar +++ b/acceptance/testdata/skills/skills-install-nonexistent-skill.txtar @@ -1,3 +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 +! exec gh skill install github/awesome-copilot nonexistent-skill-xyz --force --dir $WORK/tmp stderr 'not found' diff --git a/acceptance/testdata/skills/skills-install-pin.txtar b/acceptance/testdata/skills/skills-install-pin.txtar index 43d780e3e..7c87e4b33 100644 --- a/acceptance/testdata/skills/skills-install-pin.txtar +++ b/acceptance/testdata/skills/skills-install-pin.txtar @@ -1,7 +1,7 @@ # Install with --pin to a specific ref -exec gh skills install github/awesome-copilot git-commit --scope user --force --pin main +exec gh skill install github/awesome-copilot git-commit --scope user --force --pin main stdout 'Installed git-commit' # Install without --pin should resolve latest version -exec gh skills install github/awesome-copilot git-commit --scope user --force +exec gh skill install github/awesome-copilot git-commit --scope user --force stdout 'Installed git-commit' diff --git a/acceptance/testdata/skills/skills-install-scope.txtar b/acceptance/testdata/skills/skills-install-scope.txtar index da2df19ea..52270178a 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 writes to the git repo's .github/skills/ +# Install with --scope project writes to the git repo's .agents/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 +exec gh skill install github/awesome-copilot git-commit --scope project --force --agent github-copilot +exists $WORK/myrepo/.agents/skills/git-commit/SKILL.md # Install with --scope user writes to home directory -exec gh skills install github/awesome-copilot git-commit --scope user --force --agent github-copilot +exec gh skill install github/awesome-copilot git-commit --scope user --force --agent github-copilot 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 183f930fd..c365cb833 100644 --- a/acceptance/testdata/skills/skills-install.txtar +++ b/acceptance/testdata/skills/skills-install.txtar @@ -1,20 +1,20 @@ # Install a single skill from a public repo -exec gh skills install github/awesome-copilot git-commit --scope user --force --agent github-copilot +exec gh skill 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 +grep 'github-tree-sha' $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 +exec gh skill 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 +grep 'github-repo' $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 index 939df0ab6..7c276b8d3 100644 --- a/acceptance/testdata/skills/skills-preview-noninteractive.txtar +++ b/acceptance/testdata/skills/skills-preview-noninteractive.txtar @@ -1,3 +1,3 @@ # Preview with repo only and non-interactive should error -! exec gh skills preview github/awesome-copilot +! exec gh skill 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 index 3834c340c..be1be5244 100644 --- a/acceptance/testdata/skills/skills-preview.txtar +++ b/acceptance/testdata/skills/skills-preview.txtar @@ -1,9 +1,9 @@ # Preview renders skill content and file tree -exec gh skills preview github/awesome-copilot git-commit +exec gh skill 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 +! exec gh skill 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 index 39c0f234d..2dea21d67 100644 --- a/acceptance/testdata/skills/skills-publish-dry-run.txtar +++ b/acceptance/testdata/skills/skills-publish-dry-run.txtar @@ -1,21 +1,21 @@ # Publish dry-run from a directory with no skills/ should fail gracefully -! exec gh skills publish --dry-run $WORK +! exec gh skill 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 +exec gh skill publish --dry-run $WORK/test-repo stdout 'hello-world' # Validate alias should work identically -exec gh skills validate --dry-run $WORK/test-repo +exec gh skill 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 +exec gh skill 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 +exec gh skill publish --dry-run --fix $WORK/test-repo stdout 'hello-world' -- test-repo/skills/hello-world/SKILL.md -- diff --git a/acceptance/testdata/skills/skills-publish-lifecycle.txtar b/acceptance/testdata/skills/skills-publish-lifecycle.txtar index 0e8a03a1d..d3d6f0a3a 100644 --- a/acceptance/testdata/skills/skills-publish-lifecycle.txtar +++ b/acceptance/testdata/skills/skills-publish-lifecycle.txtar @@ -20,31 +20,31 @@ exec git commit -m 'Add test skill' exec git push origin main # Publish with a tag -exec gh skills publish --tag v0.1.0 +exec gh skill 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 +exec gh skill 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 +grep 'github-repo' $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 +exec gh skill 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 +exec gh skill 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 +exec gh skill update --dry-run --all stderr 'up to date' -- skill.md -- diff --git a/acceptance/testdata/skills/skills-search-noresults.txtar b/acceptance/testdata/skills/skills-search-noresults.txtar index 31f8293f0..425666556 100644 --- a/acceptance/testdata/skills/skills-search-noresults.txtar +++ b/acceptance/testdata/skills/skills-search-noresults.txtar @@ -1,4 +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 +exec gh skill search zzzznonexistenttotallyfakeskillxyz123 ! stdout . diff --git a/acceptance/testdata/skills/skills-search-page.txtar b/acceptance/testdata/skills/skills-search-page.txtar index 71bc6f1de..30c044f78 100644 --- a/acceptance/testdata/skills/skills-search-page.txtar +++ b/acceptance/testdata/skills/skills-search-page.txtar @@ -1,3 +1,3 @@ # Pagination returns results on page 2 -exec gh skills search copilot --page 2 +exec gh skill search copilot --page 2 stdout 'copilot' diff --git a/acceptance/testdata/skills/skills-search.txtar b/acceptance/testdata/skills/skills-search.txtar index eb4759a41..5e8c77442 100644 --- a/acceptance/testdata/skills/skills-search.txtar +++ b/acceptance/testdata/skills/skills-search.txtar @@ -1,12 +1,12 @@ # Search for skills matching a query -exec gh skills search copilot +exec gh skill search copilot stdout 'copilot' # Search with JSON output -exec gh skills search copilot --json skillName,repo --limit 1 +exec gh skill search copilot --json skillName,repo --limit 1 stdout '"skillName"' stdout '"repo"' # Search with a short query should error -! exec gh skills search a +! exec gh skill search a stderr 'at least' diff --git a/acceptance/testdata/skills/skills-update-noinstalled.txtar b/acceptance/testdata/skills/skills-update-noinstalled.txtar index 7f24291ba..7fd19541b 100644 --- a/acceptance/testdata/skills/skills-update-noinstalled.txtar +++ b/acceptance/testdata/skills/skills-update-noinstalled.txtar @@ -1,5 +1,5 @@ # Update with no installed skills should report appropriately -exec gh skills update --dry-run --all --dir $WORK/empty-dir +exec gh skill 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 index 7041c84b4..52933a5f8 100644 --- a/acceptance/testdata/skills/skills-update.txtar +++ b/acceptance/testdata/skills/skills-update.txtar @@ -1,14 +1,13 @@ # Dry-run update should find the installed skill and report status -exec gh skills update --dry-run --all --dir $WORK/skills-dir -stderr 'update' +exec gh skill update --dry-run --all --dir $WORK/skills-dir stdout 'git-commit' # Force update should re-download and rewrite files -exec gh skills update --force --all --dir $WORK/skills-dir +exec gh skill 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 'github-repo' $WORK/skills-dir/git-commit/SKILL.md ! grep 'Test skill content' $WORK/skills-dir/git-commit/SKILL.md -- skills-dir/git-commit/SKILL.md -- @@ -16,8 +15,7 @@ grep 'github-owner' $WORK/skills-dir/git-commit/SKILL.md name: git-commit description: Git commit helper metadata: - github-owner: github - github-repo: awesome-copilot + github-repo: https://github.com/github/awesome-copilot.git github-tree-sha: 0000000000000000000000000000000000000000 github-path: skills/git-commit --- diff --git a/git/client.go b/git/client.go index 22c4eff16..7f2487fce 100644 --- a/git/client.go +++ b/git/client.go @@ -715,7 +715,7 @@ func (c *Client) IsLocalGitRepo(ctx context.Context) (bool, error) { // 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) + cmd, err := c.Command(ctx, "remote", "get-url", "--", name) if err != nil { return "", err } @@ -727,13 +727,23 @@ func (c *Client) RemoteURL(ctx context.Context, name string) (string, error) { } // 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) +// Returns an error for fatal git failures (e.g. path outside repository). +func (c *Client) IsIgnored(ctx context.Context, path string) (bool, error) { + cmd, err := c.Command(ctx, "check-ignore", "-q", "--", path) if err != nil { - return false + return false, err } _, err = cmd.Output() - return err == nil + if err == nil { + return true, nil + } + // Exit 1 here means we can confirm the path is not ignored. + // Any other error is a real git error. + var exitErr *exec.ExitError + if errors.As(err, &exitErr) && exitErr.ExitCode() == 1 { + return false, nil + } + return false, err } // ShortSHA returns the first 8 characters of a SHA hash for display purposes. diff --git a/git/client_test.go b/git/client_test.go index f59b26077..7ffee2dc9 100644 --- a/git/client_test.go +++ b/git/client_test.go @@ -2164,3 +2164,123 @@ func createMockedCommandContext(t *testing.T, commands mockedCommands) commandCt return cmd } } + +func TestClientRemoteURL(t *testing.T) { + tests := []struct { + name string + cmdExitStatus int + cmdStdout string + cmdStderr string + wantCmdArgs string + wantURL string + wantErrorMsg string + }{ + { + name: "returns remote URL", + cmdStdout: "https://github.com/monalisa/skills-repo.git\n", + wantCmdArgs: "path/to/git remote get-url -- origin", + wantURL: "https://github.com/monalisa/skills-repo.git", + }, + { + name: "git error", + cmdExitStatus: 1, + cmdStderr: "fatal: No such remote 'nonexistent'", + wantCmdArgs: "path/to/git remote get-url -- nonexistent", + wantErrorMsg: "failed to run git: fatal: No such remote 'nonexistent'", + }, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + cmd, cmdCtx := createCommandContext(t, tt.cmdExitStatus, tt.cmdStdout, tt.cmdStderr) + client := Client{ + GitPath: "path/to/git", + commandContext: cmdCtx, + } + remoteName := "origin" + if tt.wantErrorMsg != "" { + remoteName = "nonexistent" + } + url, err := client.RemoteURL(context.Background(), remoteName) + assert.Equal(t, tt.wantCmdArgs, strings.Join(cmd.Args[3:], " ")) + if tt.wantErrorMsg == "" { + assert.NoError(t, err) + assert.Equal(t, tt.wantURL, url) + } else { + assert.EqualError(t, err, tt.wantErrorMsg) + } + }) + } + + // Covers the early return in RemoteURL when Command() itself fails. + // (e.g. git binary not resolvable). + t.Run("returns error when git has a fatal error", func(t *testing.T) { + t.Setenv("PATH", "") + client := Client{} + _, err := client.RemoteURL(context.Background(), "origin") + assert.Error(t, err) + }) +} + +func TestClientIsIgnored(t *testing.T) { + tests := []struct { + name string + cmdExitStatus int + cmdStdout string + cmdStderr string + wantCmdArgs string + wantIgnored bool + wantErr bool + }{ + { + name: "path is ignored", + wantCmdArgs: "path/to/git check-ignore -q -- .github/skills", + wantIgnored: true, + }, + { + name: "path is not ignored", + cmdExitStatus: 1, + wantCmdArgs: "path/to/git check-ignore -q -- .github/skills", + wantIgnored: false, + }, + { + name: "fatal git error", + cmdExitStatus: 128, + cmdStderr: "fatal: not a git repository", + wantCmdArgs: "path/to/git check-ignore -q -- .github/skills", + wantIgnored: false, + wantErr: true, + }, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + cmd, cmdCtx := createCommandContext(t, tt.cmdExitStatus, tt.cmdStdout, tt.cmdStderr) + client := Client{ + GitPath: "path/to/git", + commandContext: cmdCtx, + } + ignored, err := client.IsIgnored(context.Background(), ".github/skills") + assert.Equal(t, tt.wantCmdArgs, strings.Join(cmd.Args[3:], " ")) + assert.Equal(t, tt.wantIgnored, ignored) + if tt.wantErr { + assert.Error(t, err) + } else { + assert.NoError(t, err) + } + }) + } + + // Covers the early return in IsIgnored when Command() itself fails + // (e.g. git binary not resolvable). + t.Run("returns error when git has a fatal error", func(t *testing.T) { + t.Setenv("PATH", "") + client := Client{} + ignored, err := client.IsIgnored(context.Background(), ".github/skills") + assert.False(t, ignored) + assert.Error(t, err) + }) +} + +func TestShortSHA(t *testing.T) { + assert.Equal(t, "abc123de", ShortSHA("abc123def456789")) + assert.Equal(t, "short", ShortSHA("short")) +} diff --git a/go.mod b/go.mod index 615b1ebf4..0fc0b1a5e 100644 --- a/go.mod +++ b/go.mod @@ -57,6 +57,7 @@ require ( github.com/zalando/go-keyring v0.2.8 golang.org/x/crypto v0.50.0 golang.org/x/sync v0.20.0 + golang.org/x/sys v0.43.0 golang.org/x/term v0.42.0 golang.org/x/text v0.36.0 google.golang.org/grpc v1.80.0 @@ -182,7 +183,6 @@ require ( go.yaml.in/yaml/v3 v3.0.4 // indirect golang.org/x/mod v0.34.0 // indirect golang.org/x/net v0.53.0 // indirect - golang.org/x/sys v0.43.0 // indirect golang.org/x/tools v0.43.0 // indirect google.golang.org/genproto/googleapis/api v0.0.0-20260316180232-0b37fe3546d5 // indirect google.golang.org/genproto/googleapis/rpc v0.0.0-20260316180232-0b37fe3546d5 // indirect diff --git a/internal/flock/flock.go b/internal/flock/flock.go new file mode 100644 index 000000000..6d5af9f01 --- /dev/null +++ b/internal/flock/flock.go @@ -0,0 +1,8 @@ +package flock + +import "errors" + +// ErrLocked is returned when the file is already locked by another process. +// Callers can check for this to distinguish contention from permanent errors. +// This is intended to be an OS-agnostic sentinel error. +var ErrLocked = errors.New("file is locked by another process") diff --git a/internal/flock/flock_test.go b/internal/flock/flock_test.go new file mode 100644 index 000000000..69b3a73b5 --- /dev/null +++ b/internal/flock/flock_test.go @@ -0,0 +1,99 @@ +package flock_test + +import ( + "os" + "path/filepath" + "testing" + + "github.com/cli/cli/v2/internal/flock" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +func TestTryLock(t *testing.T) { + tests := []struct { + name string + setup func(t *testing.T) string // returns lock path + wantErr error + verify func(t *testing.T, f *os.File) + }{ + { + name: "acquires lock and returns writable file handle", + setup: func(t *testing.T) string { + return filepath.Join(t.TempDir(), "test.lock") + }, + verify: func(t *testing.T, f *os.File) { + t.Helper() + _, err := f.WriteString("hello") + require.NoError(t, err) + _, err = f.Seek(0, 0) + require.NoError(t, err) + buf := make([]byte, 5) + n, err := f.Read(buf) + assert.NoError(t, err) + assert.Equal(t, "hello", string(buf[:n])) + }, + }, + { + name: "creates lock file if it does not exist", + setup: func(t *testing.T) string { + dir := filepath.Join(t.TempDir(), "subdir") + require.NoError(t, os.MkdirAll(dir, 0o755)) + return filepath.Join(dir, "new.lock") + }, + verify: func(t *testing.T, f *os.File) { + t.Helper() + _, err := os.Stat(f.Name()) + assert.NoError(t, err) + }, + }, + { + name: "second lock on same path returns ErrLocked", + setup: func(t *testing.T) string { + lockPath := filepath.Join(t.TempDir(), "contended.lock") + _, unlock, err := flock.TryLock(lockPath) + require.NoError(t, err) + t.Cleanup(unlock) + return lockPath + }, + wantErr: flock.ErrLocked, + }, + { + name: "lock succeeds after unlock", + setup: func(t *testing.T) string { + lockPath := filepath.Join(t.TempDir(), "reuse.lock") + _, unlock, err := flock.TryLock(lockPath) + require.NoError(t, err) + unlock() + return lockPath + }, + }, + { + name: "fails on non-existent directory", + setup: func(t *testing.T) string { + return filepath.Join(t.TempDir(), "no", "such", "dir", "test.lock") + }, + wantErr: os.ErrNotExist, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + lockPath := tt.setup(t) + + f, unlock, err := flock.TryLock(lockPath) + if tt.wantErr != nil { + require.ErrorIs(t, err, tt.wantErr) + return + } + + require.NoError(t, err) + require.NotNil(t, f) + defer unlock() + + if tt.verify != nil { + tt.verify(t, f) + } + }) + } +} diff --git a/internal/flock/flock_unix.go b/internal/flock/flock_unix.go new file mode 100644 index 000000000..73f8b1557 --- /dev/null +++ b/internal/flock/flock_unix.go @@ -0,0 +1,32 @@ +//go:build !windows + +package flock + +import ( + "errors" + "os" + "syscall" +) + +// TryLock attempts to acquire an exclusive, non-blocking flock on the given path. +// Returns the locked file and an unlock function on success. The caller should +// read/write through the returned file to avoid platform differences with +// mandatory locking on Windows. +// Returns ErrLocked if the file is already locked by another process. +func TryLock(path string) (f *os.File, unlock func(), err error) { + f, err = os.OpenFile(path, os.O_CREATE|os.O_RDWR, 0o644) + if err != nil { + return nil, nil, err + } + if err := syscall.Flock(int(f.Fd()), syscall.LOCK_EX|syscall.LOCK_NB); err != nil { + _ = f.Close() + if errors.Is(err, syscall.EWOULDBLOCK) { + return nil, nil, ErrLocked + } + return nil, nil, err + } + return f, func() { + _ = syscall.Flock(int(f.Fd()), syscall.LOCK_UN) + _ = f.Close() + }, nil +} diff --git a/internal/flock/flock_windows.go b/internal/flock/flock_windows.go new file mode 100644 index 000000000..4795af083 --- /dev/null +++ b/internal/flock/flock_windows.go @@ -0,0 +1,41 @@ +//go:build windows + +package flock + +import ( + "errors" + "os" + + "golang.org/x/sys/windows" +) + +// TryLock attempts to acquire an exclusive, non-blocking lock on the given path. +// Returns the locked file and an unlock function on success. The caller should +// read/write through the returned file to avoid Windows mandatory lock conflicts. +// Returns ErrLocked if the file is already locked by another process. +func TryLock(path string) (f *os.File, unlock func(), err error) { + f, err = os.OpenFile(path, os.O_CREATE|os.O_RDWR, 0o644) + if err != nil { + return nil, nil, err + } + ol := new(windows.Overlapped) + handle := windows.Handle(f.Fd()) + err = windows.LockFileEx( + handle, + windows.LOCKFILE_EXCLUSIVE_LOCK|windows.LOCKFILE_FAIL_IMMEDIATELY, + 0, + 1, 0, + ol, + ) + if err != nil { + _ = f.Close() + if errors.Is(err, windows.ERROR_LOCK_VIOLATION) { + return nil, nil, ErrLocked + } + return nil, nil, err + } + return f, func() { + _ = windows.UnlockFileEx(handle, 0, 1, 0, ol) + _ = f.Close() + }, nil +} diff --git a/internal/skills/discovery/discovery.go b/internal/skills/discovery/discovery.go index 4e54fd5e3..84f2aa596 100644 --- a/internal/skills/discovery/discovery.go +++ b/internal/skills/discovery/discovery.go @@ -6,12 +6,15 @@ import ( "fmt" "io" "net/http" + "net/url" "os" "path" "path/filepath" "regexp" + "sort" "strings" "sync" + "sync/atomic" "github.com/cli/cli/v2/api" "github.com/cli/cli/v2/internal/skills/frontmatter" @@ -21,6 +24,17 @@ import ( // 1-64 chars, lowercase alphanumeric + hyphens, no leading/trailing/consecutive hyphens. var specNamePattern = regexp.MustCompile(`^[a-z0-9]([a-z0-9-]*[a-z0-9])?$`) +// TreeTooLargeError is returned when a repository's git tree exceeds the +// GitHub API truncation limit and full skill discovery is not possible. +type TreeTooLargeError struct { + Owner string + Repo string +} + +func (e *TreeTooLargeError) Error() string { + return fmt.Sprintf("repository tree for %s/%s is too large for full discovery", e.Owner, e.Repo) +} + // safeNamePattern matches names that are safe for filesystem use during discovery. // Allows letters (any case), numbers, hyphens, underscores, dots, and spaces. // Must start with a letter or number. This matches copilot-agent-runtime's SKILL_NAME_REGEX. @@ -127,7 +141,7 @@ type repoResponse struct { } // ResolveRef determines the git ref to use for a given owner/repo. -// Priority: explicit version → latest release tag → default branch. +// Priority: explicit version > latest release tag > default branch. func ResolveRef(client *api.Client, host, owner, repo, version string) (*ResolvedRef, error) { if version != "" { return resolveExplicitRef(client, host, owner, repo, version) @@ -166,19 +180,27 @@ func resolveExplicitRef(client *api.Client, host, owner, repo, ref string) (*Res } // Short name: try branch first, then tag, then commit SHA. + // Only fall through on 404 (not found); surface other errors + // (403, 500, network) immediately to avoid masking real failures. if resolved, err := resolveBranchRef(client, host, owner, repo, ref); err == nil { return resolved, nil + } else if !isNotFound(err) { + return nil, err } if resolved, err := resolveTagRef(client, host, owner, repo, ref); err == nil { return resolved, nil + } else if !isNotFound(err) { + return nil, err } - commitPath := fmt.Sprintf("repos/%s/%s/commits/%s", owner, repo, ref) + commitPath := fmt.Sprintf("repos/%s/%s/commits/%s", url.PathEscape(owner), url.PathEscape(repo), url.PathEscape(ref)) var commitResp struct { SHA string `json:"sha"` } if err := client.REST(host, "GET", commitPath, nil, &commitResp); err == nil { return &ResolvedRef{Ref: commitResp.SHA, SHA: commitResp.SHA}, nil + } else if !isNotFound(err) { + return nil, err } return nil, fmt.Errorf("ref %q not found as branch, tag, or commit in %s/%s", ref, owner, repo) @@ -187,7 +209,7 @@ func resolveExplicitRef(client *api.Client, host, owner, repo, ref string) (*Res // resolveTagRef looks up a tag by short name and returns a fully qualified ref. // For annotated tags, the tag object is dereferenced to obtain the commit SHA. func resolveTagRef(client *api.Client, host, owner, repo, tag string) (*ResolvedRef, error) { - tagPath := fmt.Sprintf("repos/%s/%s/git/ref/tags/%s", owner, repo, tag) + tagPath := fmt.Sprintf("repos/%s/%s/git/ref/tags/%s", url.PathEscape(owner), url.PathEscape(repo), url.PathEscape(tag)) var refResp struct { Object struct { SHA string `json:"sha"` @@ -199,7 +221,7 @@ func resolveTagRef(client *api.Client, host, owner, repo, tag string) (*Resolved } sha := refResp.Object.SHA if refResp.Object.Type == "tag" { - derefPath := fmt.Sprintf("repos/%s/%s/git/tags/%s", owner, repo, sha) + derefPath := fmt.Sprintf("repos/%s/%s/git/tags/%s", url.PathEscape(owner), url.PathEscape(repo), url.PathEscape(sha)) var tagResp struct { Object struct { SHA string `json:"sha"` @@ -215,7 +237,7 @@ func resolveTagRef(client *api.Client, host, owner, repo, tag string) (*Resolved // resolveBranchRef looks up a branch by short name and returns a fully qualified ref. func resolveBranchRef(client *api.Client, host, owner, repo, branch string) (*ResolvedRef, error) { - refPath := fmt.Sprintf("repos/%s/%s/git/ref/heads/%s", owner, repo, branch) + refPath := fmt.Sprintf("repos/%s/%s/git/ref/heads/%s", url.PathEscape(owner), url.PathEscape(repo), url.PathEscape(branch)) var refResp struct { Object struct { SHA string `json:"sha"` @@ -227,6 +249,12 @@ func resolveBranchRef(client *api.Client, host, owner, repo, branch string) (*Re return &ResolvedRef{Ref: "refs/heads/" + branch, SHA: refResp.Object.SHA}, nil } +// isNotFound returns true if the error is an HTTP 404 response. +func isNotFound(err error) bool { + var httpErr api.HTTPError + return errors.As(err, &httpErr) && httpErr.StatusCode == http.StatusNotFound +} + // noReleasesError signals that the repository has no usable releases, // which is the only case where ResolveRef should fall back to the // default branch. @@ -237,16 +265,15 @@ type noReleasesError struct { func (e *noReleasesError) Error() string { return e.reason } func resolveLatestRelease(client *api.Client, host, owner, repo string) (*ResolvedRef, error) { - apiPath := fmt.Sprintf("repos/%s/%s/releases/latest", owner, repo) + apiPath := fmt.Sprintf("repos/%s/%s/releases/latest", url.PathEscape(owner), url.PathEscape(repo)) var release releaseResponse if err := client.REST(host, "GET", apiPath, nil, &release); err != nil { - // A 404 means the repository has no releases — this is the + // A 404 means the repository has no releases. This is the // only case where falling back to the default branch is safe. // Any other HTTP error (403, 500, …) or network failure is // returned as-is so ResolveRef surfaces it rather than // silently falling back. - var httpErr api.HTTPError - if errors.As(err, &httpErr) && httpErr.StatusCode == http.StatusNotFound { + if isNotFound(err) { return nil, &noReleasesError{reason: fmt.Sprintf("no releases found for %s/%s", owner, repo)} } return nil, fmt.Errorf("could not fetch latest release: %w", err) @@ -258,14 +285,14 @@ func resolveLatestRelease(client *api.Client, host, owner, repo string) (*Resolv } func resolveDefaultBranch(client *api.Client, host, owner, repo string) (*ResolvedRef, error) { - apiPath := fmt.Sprintf("repos/%s/%s", owner, repo) + apiPath := fmt.Sprintf("repos/%s/%s", url.PathEscape(owner), url.PathEscape(repo)) var repoResp repoResponse if err := client.REST(host, "GET", apiPath, nil, &repoResp); err != nil { return nil, fmt.Errorf("could not determine default branch: %w", err) } branch := repoResp.DefaultBranch if branch == "" { - branch = "main" + return nil, fmt.Errorf("could not determine default branch for %s/%s", owner, repo) } return resolveBranchRef(client, host, owner, repo, branch) } @@ -333,18 +360,14 @@ func matchSkillConventions(entry treeEntry) *skillMatch { // DiscoverSkills finds all skills in a repository at the given commit SHA. 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) + apiPath := fmt.Sprintf("repos/%s/%s/git/trees/%s?recursive=true", url.PathEscape(owner), url.PathEscape(repo), url.PathEscape(commitSHA)) var tree treeResponse if err := client.REST(host, "GET", apiPath, nil, &tree); err != nil { return nil, fmt.Errorf("could not fetch repository tree: %w", err) } if tree.Truncated { - return nil, fmt.Errorf( - "repository tree for %s/%s is too large for full discovery\n"+ - " Use path-based install instead: gh skill install %s/%s skills/", - owner, repo, owner, repo, - ) + return nil, &TreeTooLargeError{Owner: owner, Repo: repo} } treeSHAs := make(map[string]string) @@ -393,6 +416,10 @@ func DiscoverSkills(client *api.Client, host, owner, repo, commitSHA string) ([] }) } + sort.SliceStable(skills, func(i, j int) bool { + return skills[i].DisplayName() < skills[j].DisplayName() + }) + return skills, nil } @@ -425,33 +452,31 @@ func FetchDescriptionsConcurrent(client *api.Client, host, owner, repo string, s } const maxWorkers = 10 - sem := make(chan struct{}, maxWorkers) - var mu sync.Mutex - done := 0 - var wg sync.WaitGroup - for i := range skills { - if skills[i].Description != "" { - continue - } - wg.Add(1) - go func(idx int) { - defer wg.Done() - sem <- struct{}{} - defer func() { <-sem }() + var done atomic.Int32 - desc := fetchDescription(client, host, owner, repo, &skills[idx]) + jobs := make(chan *Skill) - mu.Lock() - skills[idx].Description = desc - done++ - d := done - mu.Unlock() - if onProgress != nil { - onProgress(d, total) + workers := min(maxWorkers, total) + for range workers { + wg.Go(func() { + for s := range jobs { + s.Description = fetchDescription(client, host, owner, repo, s) + + d := int(done.Add(1)) + if onProgress != nil { + onProgress(d, total) + } } - }(i) + }) } + + for i := range skills { + if skills[i].Description == "" { + jobs <- &skills[i] + } + } + close(jobs) wg.Wait() } @@ -466,7 +491,7 @@ func DiscoverSkillByPath(client *api.Client, host, owner, repo, commitSHA, skill } parentPath := path.Dir(skillPath) - apiPath := fmt.Sprintf("repos/%s/%s/contents/%s?ref=%s", owner, repo, parentPath, commitSHA) + apiPath := fmt.Sprintf("repos/%s/%s/contents/%s?ref=%s", url.PathEscape(owner), url.PathEscape(repo), parentPath, commitSHA) var contents []struct { Name string `json:"name"` @@ -489,7 +514,7 @@ func DiscoverSkillByPath(client *api.Client, host, owner, repo, commitSHA, skill return nil, fmt.Errorf("skill directory %q not found in %s/%s", skillPath, owner, repo) } - skillTreePath := fmt.Sprintf("repos/%s/%s/git/trees/%s", owner, repo, treeSHA) + skillTreePath := fmt.Sprintf("repos/%s/%s/git/trees/%s", url.PathEscape(owner), url.PathEscape(repo), url.PathEscape(treeSHA)) var skillTree treeResponse if err := client.REST(host, "GET", skillTreePath, nil, &skillTree); err != nil { return nil, fmt.Errorf("could not read skill directory: %w", err) @@ -528,15 +553,15 @@ func DiscoverSkillByPath(client *api.Client, host, owner, repo, commitSHA, skill // DiscoverSkillFiles returns all file paths belonging to a skill directory // by fetching the skill's subtree directly using its tree SHA. 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) + apiPath := fmt.Sprintf("repos/%s/%s/git/trees/%s?recursive=true", url.PathEscape(owner), url.PathEscape(repo), url.PathEscape(treeSHA)) var tree treeResponse if err := client.REST(host, "GET", apiPath, nil, &tree); err != nil { return nil, fmt.Errorf("could not fetch skill tree: %w", err) } if tree.Truncated { - // Recursive fetch was truncated — fall back to walking subtrees individually. - return walkTree(client, host, owner, repo, treeSHA, skillPath) + // Recursive fetch was truncated. Fall back to walking subtrees individually. + return walkTree(client, host, owner, repo, treeSHA, skillPath, 0) } var files []SkillFile @@ -556,7 +581,7 @@ func DiscoverSkillFiles(client *api.Client, host, owner, repo, treeSHA, skillPat // ListSkillFiles returns all files in a skill directory as public SkillFile // structs with paths relative to the skill root. 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) + apiPath := fmt.Sprintf("repos/%s/%s/git/trees/%s?recursive=true", url.PathEscape(owner), url.PathEscape(repo), url.PathEscape(treeSHA)) var tree treeResponse if err := client.REST(host, "GET", apiPath, nil, &tree); err != nil { return nil, fmt.Errorf("could not fetch skill tree: %w", err) @@ -564,7 +589,7 @@ func ListSkillFiles(client *api.Client, host, owner, repo, treeSHA string) ([]Sk if tree.Truncated { // Fall back to non-recursive traversal when the tree is too large. - return walkTree(client, host, owner, repo, treeSHA, "") + return walkTree(client, host, owner, repo, treeSHA, "", 0) } var files []SkillFile @@ -580,10 +605,18 @@ func ListSkillFiles(client *api.Client, host, owner, repo, treeSHA string) ([]Sk return files, nil } +// maxTreeDepth bounds the recursion in walkTree to prevent unbounded +// API calls on deeply nested repositories. +const maxTreeDepth = 20 + // walkTree enumerates files by fetching each tree level individually, -// avoiding the truncation limit of the recursive tree API. -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) +// avoiding the truncation limit of the recursive tree API. Recursion +// depth is bounded by maxTreeDepth to prevent unbounded API calls. +func walkTree(client *api.Client, host, owner, repo, sha, prefix string, depth int) ([]SkillFile, error) { + if depth > maxTreeDepth { + return nil, fmt.Errorf("tree depth exceeds %d levels at %s", maxTreeDepth, prefix) + } + apiPath := fmt.Sprintf("repos/%s/%s/git/trees/%s", url.PathEscape(owner), url.PathEscape(repo), url.PathEscape(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) @@ -599,7 +632,7 @@ func walkTree(client *api.Client, host, owner, repo, sha, prefix string) ([]Skil case "blob": files = append(files, SkillFile{Path: entryPath, SHA: entry.SHA, Size: entry.Size}) case "tree": - sub, err := walkTree(client, host, owner, repo, entry.SHA, entryPath) + sub, err := walkTree(client, host, owner, repo, entry.SHA, entryPath, depth+1) if err != nil { return nil, err } @@ -611,7 +644,7 @@ func walkTree(client *api.Client, host, owner, repo, sha, prefix string) ([]Skil // FetchBlob retrieves the content of a blob by SHA. func FetchBlob(client *api.Client, host, owner, repo, sha string) (string, error) { - apiPath := fmt.Sprintf("repos/%s/%s/git/blobs/%s", owner, repo, sha) + apiPath := fmt.Sprintf("repos/%s/%s/git/blobs/%s", url.PathEscape(owner), url.PathEscape(repo), url.PathEscape(sha)) var blob blobResponse if err := client.REST(host, "GET", apiPath, nil, &blob); err != nil { return "", fmt.Errorf("could not fetch blob: %w", err) diff --git a/internal/skills/discovery/discovery_test.go b/internal/skills/discovery/discovery_test.go index 3bc719ae8..2de7ef683 100644 --- a/internal/skills/discovery/discovery_test.go +++ b/internal/skills/discovery/discovery_test.go @@ -438,7 +438,7 @@ func TestResolveRef(t *testing.T) { wantSHA: "fallback-sha", }, { - name: "empty default_branch falls back to main", + name: "empty default_branch returns error", stubs: func(reg *httpmock.Registry) { reg.Register( httpmock.REST("GET", "repos/monalisa/octocat-skills/releases/latest"), @@ -446,14 +446,41 @@ func TestResolveRef(t *testing.T) { reg.Register( httpmock.REST("GET", "repos/monalisa/octocat-skills"), httpmock.JSONResponse(map[string]interface{}{"default_branch": ""})) + }, + wantErr: "could not determine default branch", + }, + { + name: "short name with server error on branch lookup does not fall through", + version: "main", + stubs: func(reg *httpmock.Registry) { reg.Register( httpmock.REST("GET", "repos/monalisa/octocat-skills/git/ref/heads/main"), - httpmock.JSONResponse(map[string]interface{}{ - "object": map[string]interface{}{"sha": "main-sha"}, - })) + httpmock.StatusStringResponse(500, "server error")) }, - wantRef: "refs/heads/main", - wantSHA: "main-sha", + wantErr: `branch "main" not found in monalisa/octocat-skills`, + }, + { + name: "short name with forbidden error on branch lookup does not fall through", + version: "develop", + stubs: func(reg *httpmock.Registry) { + reg.Register( + httpmock.REST("GET", "repos/monalisa/octocat-skills/git/ref/heads/develop"), + httpmock.StatusStringResponse(403, "forbidden")) + }, + wantErr: `branch "develop" not found in monalisa/octocat-skills`, + }, + { + name: "short name with server error on tag lookup does not fall through", + version: "v5.0", + stubs: func(reg *httpmock.Registry) { + reg.Register( + httpmock.REST("GET", "repos/monalisa/octocat-skills/git/ref/heads/v5.0"), + httpmock.StatusStringResponse(404, "not found")) + reg.Register( + httpmock.REST("GET", "repos/monalisa/octocat-skills/git/ref/tags/v5.0"), + httpmock.StatusStringResponse(500, "server error")) + }, + wantErr: `tag "v5.0" not found in monalisa/octocat-skills`, }, } for _, tt := range tests { diff --git a/internal/skills/installer/installer.go b/internal/skills/installer/installer.go index 5fdf99ce0..0ac9e182c 100644 --- a/internal/skills/installer/installer.go +++ b/internal/skills/installer/installer.go @@ -8,6 +8,7 @@ import ( "path/filepath" "strings" "sync" + "sync/atomic" "github.com/cli/cli/v2/api" "github.com/cli/cli/v2/git" @@ -86,31 +87,34 @@ func Install(opts *Options) (*Result, error) { opts.OnProgress(0, total) } - sem := make(chan struct{}, maxConcurrency) + type job struct { + idx int + skill discovery.Skill + } + jobs := make(chan job) + results := make([]skillResult, total) var wg sync.WaitGroup - var mu sync.Mutex - done := 0 + var done atomic.Int32 - for i, skill := range opts.Skills { - wg.Add(1) - go func(idx int, s discovery.Skill) { - defer wg.Done() - sem <- struct{}{} - defer func() { <-sem }() + workers := min(maxConcurrency, total) + for range workers { + wg.Go(func() { + for j := range jobs { + err := installSkill(opts, j.skill, targetDir) + results[j.idx] = skillResult{name: j.skill.InstallName(), err: err} - err := installSkill(opts, s, targetDir) - results[idx] = skillResult{name: s.InstallName(), err: err} - - if opts.OnProgress != nil { - mu.Lock() - done++ - d := done - mu.Unlock() - opts.OnProgress(d, total) + if opts.OnProgress != nil { + opts.OnProgress(int(done.Add(1)), total) + } } - }(i, skill) + }) } + + for i, s := range opts.Skills { + jobs <- job{idx: i, skill: s} + } + close(jobs) wg.Wait() var installed []string diff --git a/internal/skills/installer/installer_test.go b/internal/skills/installer/installer_test.go index 85c8bcf18..6334add85 100644 --- a/internal/skills/installer/installer_test.go +++ b/internal/skills/installer/installer_test.go @@ -134,7 +134,7 @@ func TestInstallLocal(t *testing.T) { }, verify: func(t *testing.T, destDir string) { t.Helper() - _, err := os.Stat(filepath.Join(destDir, ".github", "skills", "code-review", "SKILL.md")) + _, err := os.Stat(filepath.Join(destDir, ".agents", "skills", "code-review", "SKILL.md")) assert.NoError(t, err) }, }, diff --git a/internal/skills/lockfile/lockfile.go b/internal/skills/lockfile/lockfile.go index 3a6ccd893..42d2abb34 100644 --- a/internal/skills/lockfile/lockfile.go +++ b/internal/skills/lockfile/lockfile.go @@ -2,10 +2,14 @@ package lockfile import ( "encoding/json" + "errors" "fmt" + "io" "os" "path/filepath" "time" + + "github.com/cli/cli/v2/internal/flock" ) const ( @@ -43,61 +47,68 @@ func lockfilePath() (string, error) { return filepath.Join(home, agentsDir, lockFile), nil } -// 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 := lockfilePath() - if err != nil { - return newFile(), nil //nolint:nilerr // graceful: no home dir means fresh state +// readFrom loads the lock file from an open file handle. +// Returns an empty file if the content is empty, corrupt, or incompatible. +func readFrom(f *os.File) (*file, error) { + if _, err := f.Seek(0, 0); err != nil { + return nil, fmt.Errorf("could not seek lock file: %w", err) } - - data, err := os.ReadFile(lockPath) + data, err := io.ReadAll(f) if err != nil { - if os.IsNotExist(err) { - return newFile(), nil - } return nil, fmt.Errorf("could not read lock file: %w", err) } - - var f file - if err := json.Unmarshal(data, &f); err != nil { - return newFile(), nil //nolint:nilerr // graceful: corrupt file means fresh state - } - - if f.Version != lockVersion || f.Skills == nil { + if len(data) == 0 { return newFile(), nil } - return &f, nil + var lf file + if err := json.Unmarshal(data, &lf); err != nil { + return newFile(), nil //nolint:nilerr // graceful: corrupt file means fresh state + } + + if lf.Version != lockVersion || lf.Skills == nil { + return newFile(), nil + } + + return &lf, nil } -// write persists the lock file to disk. -func write(f *file) error { - lockPath, err := lockfilePath() +// writeTo persists the lock file through an open file handle. +func writeTo(f *os.File, lf *file) error { + data, err := json.MarshalIndent(lf, "", " ") if err != nil { return err } - if err := os.MkdirAll(filepath.Dir(lockPath), 0o755); err != nil { + if _, err := f.Seek(0, 0); err != nil { return err } - - data, err := json.MarshalIndent(f, "", " ") - if err != nil { + if err := f.Truncate(0); err != nil { return err } - - return os.WriteFile(lockPath, data, 0o644) + _, err = f.Write(data) + return err } // RecordInstall adds or updates a skill entry in the lock file. // It uses a file-based lock to prevent concurrent read-modify-write races // when multiple install processes run simultaneously. func RecordInstall(skillName, owner, repo, skillPath, treeSHA, pinnedRef string) error { - unlock := acquireLock() + lockPath, err := lockfilePath() + if err != nil { + return err + } + if err := os.MkdirAll(filepath.Dir(lockPath), 0o755); err != nil { + return fmt.Errorf("could not create lock directory: %w", err) + } + + lockedFile, unlock, err := acquireFLock() + if err != nil { + return err + } defer unlock() - f, err := read() + f, err := readFrom(lockedFile) if err != nil { return err } @@ -121,7 +132,7 @@ func RecordInstall(skillName, owner, repo, skillPath, treeSHA, pinnedRef string) PinnedRef: pinnedRef, } - return write(f) + return writeTo(lockedFile, f) } func newFile() *file { @@ -132,44 +143,35 @@ func newFile() *file { } var ( - lockRetries = 30 - lockRetryInterval = 100 * time.Millisecond + lockAttempts = 30 + lockAttemptDelay = 100 * time.Millisecond ) -// acquireLock creates an exclusive lock file to serialize concurrent access. -// Returns an unlock function. If locking fails after retries, it proceeds -// unlocked rather than blocking the user indefinitely. -func acquireLock() (unlock func()) { - lockPath, pathErr := lockfilePath() - if pathErr != nil { - return func() {} - } - lkPath := lockPath + ".lk" - - // Ensure the parent directory exists (fresh machine may lack ~/.agents). - if err := os.MkdirAll(filepath.Dir(lockPath), 0o755); err != nil { - return func() {} +// acquireFLock attempts to acquire an exclusive file lock to serialize concurrent access. +// Returns the locked file handle and an unlock function, or an error if the lock +// cannot be acquired. The caller should read/write through the returned file to +// avoid Windows mandatory lock conflicts. +func acquireFLock() (f *os.File, unlock func(), err error) { + lockPath, err := lockfilePath() + if err != nil { + return nil, nil, fmt.Errorf("could not determine lock path: %w", err) } - for range lockRetries { - f, createErr := os.OpenFile(lkPath, os.O_CREATE|os.O_EXCL|os.O_WRONLY, 0o644) - if createErr == nil { - f.Close() - return func() { os.Remove(lkPath) } + var lastErr error + for attempt := range lockAttempts { + f, unlock, err := flock.TryLock(lockPath) + if err == nil { + return f, unlock, nil } - // Only retry when the lock file already exists (concurrent process). - // For other errors (permission denied, invalid path, etc.) give up immediately. - if !os.IsExist(createErr) { - return func() {} + lastErr = err + + if !errors.Is(err, flock.ErrLocked) { + return nil, nil, err } - // Break stale locks older than 30s (e.g. from a crashed process). - if info, statErr := os.Stat(lkPath); statErr == nil && time.Since(info.ModTime()) > 30*time.Second { - os.Remove(lkPath) - continue + if attempt < lockAttempts-1 { + time.Sleep(lockAttemptDelay) } - time.Sleep(lockRetryInterval) } - // Best-effort: proceed without lock. - return func() {} + return nil, nil, fmt.Errorf("could not acquire lock after %d attempts: %w", lockAttempts, lastErr) } diff --git a/internal/skills/lockfile/lockfile_test.go b/internal/skills/lockfile/lockfile_test.go index d4a44f76d..d68e9a8f1 100644 --- a/internal/skills/lockfile/lockfile_test.go +++ b/internal/skills/lockfile/lockfile_test.go @@ -5,8 +5,8 @@ import ( "os" "path/filepath" "testing" - "time" + "github.com/cli/cli/v2/internal/flock" "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" ) @@ -23,13 +23,14 @@ func setupTestHome(t *testing.T) string { func TestRecordInstall(t *testing.T) { tests := []struct { name string - setup func(t *testing.T) // optional pre-existing state + setup func(t *testing.T) skill string owner string repo string skillPath string treeSHA string pinnedRef string + wantErr bool verify func(t *testing.T, lockPath string) }{ { @@ -87,63 +88,31 @@ func TestRecordInstall(t *testing.T) { }, }, { - name: "succeeds despite stale lock file", + name: "returns error when lock cannot be acquired", setup: func(t *testing.T) { t.Helper() - lockPath, err := lockfilePath() - require.NoError(t, err) - require.NoError(t, os.MkdirAll(filepath.Dir(lockPath), 0o755)) - lkPath := lockPath + ".lk" - f, err := os.Create(lkPath) - require.NoError(t, err) - f.Close() - staleTime := time.Now().Add(-60 * time.Second) - require.NoError(t, os.Chtimes(lkPath, staleTime, staleTime)) - }, - 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 := readTestLockfile(t, lockPath) - require.Contains(t, f.Skills, "code-review") - _, err := os.Stat(lockPath + ".lk") - assert.True(t, os.IsNotExist(err), "stale lock should be removed after RecordInstall") - }, - }, - { - name: "proceeds without lock after retries exhausted", - setup: func(t *testing.T) { - t.Helper() - // Reduce retries to avoid 3s wait in tests. - origRetries := lockRetries - origInterval := lockRetryInterval - lockRetries = 1 - lockRetryInterval = 0 + origAttempts := lockAttempts + origDelay := lockAttemptDelay + lockAttempts = 1 + lockAttemptDelay = 0 t.Cleanup(func() { - lockRetries = origRetries - lockRetryInterval = origInterval + lockAttempts = origAttempts + lockAttemptDelay = origDelay }) - // Create a fresh (non-stale) lock file that won't be broken. + // Hold a real flock so acquireFLock fails. lockPath, err := lockfilePath() require.NoError(t, err) require.NoError(t, os.MkdirAll(filepath.Dir(lockPath), 0o755)) - f, err := os.Create(lockPath + ".lk") + _, unlock, err := flock.TryLock(lockPath) require.NoError(t, err) - f.Close() + t.Cleanup(unlock) }, 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 := readTestLockfile(t, lockPath) - require.Contains(t, f.Skills, "code-review", "should succeed best-effort without lock") - }, + wantErr: true, }, { name: "recovers from corrupt lockfile", @@ -198,6 +167,10 @@ func TestRecordInstall(t *testing.T) { } err := RecordInstall(tt.skill, tt.owner, tt.repo, tt.skillPath, tt.treeSHA, tt.pinnedRef) + if tt.wantErr { + require.Error(t, err) + return + } require.NoError(t, err) tt.verify(t, lockPath) }) diff --git a/internal/skills/registry/registry.go b/internal/skills/registry/registry.go index a8fdc5993..b112d361a 100644 --- a/internal/skills/registry/registry.go +++ b/internal/skills/registry/registry.go @@ -28,6 +28,8 @@ const ( ScopeProject Scope = "project" ScopeUser Scope = "user" + DefaultAgentID = "github-copilot" + sharedProjectSkillsDir = ".agents/skills" ) @@ -144,13 +146,13 @@ func (h *AgentHost) InstallDir(scope Scope, gitRoot, homeDir string) (string, er // If repoName is non-empty, it is included in the project-scope label // for additional context. func ScopeLabels(repoName string) []string { - projectLabel := "Project — install in current repository (recommended)" + projectLabel := "Project: install in current repository (recommended)" if repoName != "" { - projectLabel = fmt.Sprintf("Project — %s (recommended)", repoName) + projectLabel = fmt.Sprintf("Project: %s (recommended)", repoName) } return []string{ projectLabel, - "Global — install in home directory (available everywhere)", + "Global: install in home directory (available everywhere)", } } diff --git a/pkg/cmd/root/root.go b/pkg/cmd/root/root.go index 262af1b78..d44ad840c 100644 --- a/pkg/cmd/root/root.go +++ b/pkg/cmd/root/root.go @@ -145,6 +145,7 @@ func NewCmdRoot(f *cmdutil.Factory, version, buildDate string) (*cobra.Command, cmd.AddCommand(codespaceCmd.NewCmdCodespace(f)) cmd.AddCommand(projectCmd.NewCmdProject(f)) cmd.AddCommand(previewCmd.NewCmdPreview(f)) + cmd.AddCommand(skillsCmd.NewCmdSkills(f)) // Root commands with standalone functionality and no subcommands cmd.AddCommand(copilotCmd.NewCmdCopilot(f, nil)) @@ -165,7 +166,6 @@ func NewCmdRoot(f *cmdutil.Factory, version, buildDate string) (*cobra.Command, cmd.AddCommand(repoCmd.NewCmdRepo(&repoResolvingCmdFactory)) cmd.AddCommand(rulesetCmd.NewCmdRuleset(&repoResolvingCmdFactory)) cmd.AddCommand(runCmd.NewCmdRun(&repoResolvingCmdFactory)) - cmd.AddCommand(skillsCmd.NewCmdSkills(f)) cmd.AddCommand(workflowCmd.NewCmdWorkflow(&repoResolvingCmdFactory)) cmd.AddCommand(labelCmd.NewCmdLabel(&repoResolvingCmdFactory)) cmd.AddCommand(cacheCmd.NewCmdCache(&repoResolvingCmdFactory)) diff --git a/pkg/cmd/skills/install/install.go b/pkg/cmd/skills/install/install.go index fc65e2f0c..fce53583d 100644 --- a/pkg/cmd/skills/install/install.go +++ b/pkg/cmd/skills/install/install.go @@ -7,7 +7,6 @@ import ( "net/http" "os" "path/filepath" - "sort" "strings" "github.com/MakeNowJust/heredoc" @@ -36,35 +35,32 @@ const ( maxSearchResults = 30 ) -// installOptions holds all dependencies and user-provided flags for the install command. -type installOptions struct { +// InstallOptions holds all dependencies and user-provided flags for the install command. +type InstallOptions struct { IO *iostreams.IOStreams HttpClient func() (*http.Client, error) Prompter prompter.Prompter GitClient *git.Client Remotes func() (ghContext.Remotes, error) - // Arguments - SkillSource string // owner/repo or local path - SkillName string // skill name, possibly with @version + SkillSource string // owner/repo or local path (when --from-local is set) + SkillName string // possibly with @version suffix + Agent string + Scope string + ScopeChanged bool // true when --scope was explicitly set + Pin string + Dir string // overrides --agent and --scope + Force bool + FromLocal bool // treat SkillSource as a local directory path - // Flags - Agent string // --agent flag - Scope string // --scope flag - ScopeChanged bool // true when --scope was explicitly set - Pin string // --pin flag - Dir string // --dir flag (overrides host+scope) - Force bool // --force flag - - // Resolved at runtime repo ghrepo.Interface // set when SkillSource is a GitHub repository - localPath string // set when SkillSource is a local directory - version string + localPath string // set when FromLocal is true + version string // parsed from SkillName@version } // NewCmdInstall creates the "skills install" command. -func NewCmdInstall(f *cmdutil.Factory, runF func(*installOptions) error) *cobra.Command { - opts := &installOptions{ +func NewCmdInstall(f *cmdutil.Factory, runF func(*InstallOptions) error) *cobra.Command { + opts := &InstallOptions{ IO: f.IOStreams, Prompter: f.Prompter, GitClient: f.GitClient, @@ -73,21 +69,21 @@ func NewCmdInstall(f *cmdutil.Factory, runF func(*installOptions) error) *cobra. } cmd := &cobra.Command{ - Use: "install []", - Short: "Install agent skills from a GitHub repository", + Use: "install [] [flags]", + Short: "Install agent skills from a GitHub repository (preview)", Long: heredoc.Docf(` Install agent skills from a GitHub repository or local directory into your local environment. Skills are placed in a host-specific directory at either project scope (inside the current git repository) or user - scope (in your home directory, available everywhere): + scope (in your home directory, available everywhere). Supported hosts + and their storage directories are (project, user): - Host Project User - GitHub Copilot .agents/skills ~/.copilot/skills - Claude Code .claude/skills ~/.claude/skills - Cursor .agents/skills ~/.cursor/skills - Codex .agents/skills ~/.codex/skills - Gemini CLI .agents/skills ~/.gemini/skills - Antigravity .agents/skills ~/.gemini/antigravity/skills + - GitHub Copilot (%[1]s.agents/skills%[1]s, %[1]s~/.copilot/skills%[1]s) + - Claude Code (%[1]s.claude/skills%[1]s, %[1]s~/.claude/skills%[1]s) + - Cursor (%[1]s.agents/skills%[1]s, %[1]s~/.cursor/skills%[1]s) + - Codex (%[1]s.agents/skills%[1]s, %[1]s~/.codex/skills%[1]s) + - Gemini CLI (%[1]s.agents/skills%[1]s, %[1]s~/.gemini/skills%[1]s) + - Antigravity (%[1]s.agents/skills%[1]s, %[1]s~/.gemini/antigravity/skills%[1]s) Use %[1]s--agent%[1]s and %[1]s--scope%[1]s to control placement, or %[1]s--dir%[1]s for a custom directory. The default scope is %[1]sproject%[1]s, and the default @@ -98,11 +94,11 @@ func NewCmdInstall(f *cmdutil.Factory, runF func(*installOptions) error) *cobra. select multiple hosts that resolve to the same destination, each skill is installed there only once. - The first argument can be a GitHub repository in %[1]sOWNER/REPO%[1]s format - or a local directory path (e.g. %[1]s.%[1]s, %[1]s./my-skills%[1]s, %[1]s~/skills%[1]s). - For local directories, skills are auto-discovered using the same - conventions as remote repositories, and files are copied (not symlinked) - with local-path tracking metadata injected into frontmatter. + The first argument is a GitHub repository in %[1]sOWNER/REPO%[1]s format. + Use %[1]s--from-local%[1]s to install from a local directory instead. + Local skills are auto-discovered using the same conventions as remote + repositories, and files are copied (not symlinked) with local-path + tracking metadata injected into frontmatter. Skills are discovered automatically using the %[1]sskills/*/SKILL.md%[1]s convention defined by the Agent Skills specification. For more information on the specification, @@ -125,12 +121,9 @@ func NewCmdInstall(f *cmdutil.Factory, runF func(*installOptions) error) *cobra. To pin to a specific version, either append %[1]s@VERSION%[1]s to the skill name or use the %[1]s--pin%[1]s flag. The version is resolved as a git tag or commit SHA. - Installed skills have GitHub tracking metadata injected into their - frontmatter (%[1]sgithub-repo%[1]s, %[1]sgithub-ref%[1]s, - %[1]sgithub-tree-sha%[1]s, %[1]sgithub-path%[1]s). This - metadata identifies the source repository and enables %[1]sgh skill update%[1]s - to detect changes — the tree SHA serves as an ETag for staleness checks. - The %[1]sgithub-repo%[1]s value is stored as a full repository URL. + Installed skills have source tracking metadata injected into their + frontmatter. This metadata identifies the source repository and + enables %[1]sgh skill update%[1]s to detect changes. When run interactively, the command prompts for any missing arguments. When run non-interactively, %[1]srepository%[1]s and a skill name are @@ -152,14 +145,11 @@ func NewCmdInstall(f *cmdutil.Factory, runF func(*installOptions) error) *cobra. # Install from a large namespaced repo by path (efficient, skips full discovery) $ gh skill install github/awesome-copilot skills/monalisa/code-review - # Install from a local directory (auto-discovers skills) - $ gh skill install ./my-skills-repo + # Install from a local directory + $ gh skill install ./my-skills-repo --from-local - # Install from current directory - $ gh skill install . - - # Install a single local skill directory - $ gh skill install ./skills/git-commit + # Install a specific local skill + $ gh skill install ./my-skills-repo git-commit --from-local # Install for Claude Code at user scope $ gh skill install github/awesome-copilot git-commit --agent claude-code --scope user @@ -170,9 +160,6 @@ func NewCmdInstall(f *cmdutil.Factory, runF func(*installOptions) error) *cobra. Aliases: []string{"add"}, Args: cobra.MaximumNArgs(2), RunE: func(cmd *cobra.Command, args []string) error { - if len(args) == 0 && !opts.IO.CanPrompt() { - return cmdutil.FlagErrorf("must specify a repository to install from") - } if len(args) >= 1 { opts.SkillSource = args[0] } @@ -182,14 +169,17 @@ func NewCmdInstall(f *cmdutil.Factory, runF func(*installOptions) error) *cobra. opts.ScopeChanged = cmd.Flags().Changed("scope") // Resolve the source type early so installRun can branch directly. - if isLocalPath(opts.SkillSource) { + if opts.FromLocal { + if opts.SkillSource == "" { + return cmdutil.FlagErrorf("--from-local requires a directory path argument") + } opts.localPath = opts.SkillSource + } else if len(args) == 0 && !opts.IO.CanPrompt() { + return cmdutil.FlagErrorf("must specify a repository to install from") } - if opts.Agent != "" { - if _, err := registry.FindByID(opts.Agent); err != nil { - return cmdutil.FlagErrorf("invalid value for --agent: %s", err) - } + if err := cmdutil.MutuallyExclusive("--from-local and --pin cannot be used together", opts.FromLocal, opts.Pin != ""); err != nil { + return err } if opts.Pin != "" && opts.SkillName != "" && strings.Contains(opts.SkillName, "@") { @@ -203,19 +193,17 @@ func NewCmdInstall(f *cmdutil.Factory, runF func(*installOptions) error) *cobra. }, } - 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 registry.AgentIDs(), cobra.ShellCompDirectiveNoFileComp - }) + cmdutil.StringEnumFlag(cmd, &opts.Agent, "agent", "", "", registry.AgentIDs(), "Target agent") 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") - cmd.Flags().StringVar(&opts.Dir, "dir", "", "install to a custom directory (overrides --agent and --scope)") - cmd.Flags().BoolVarP(&opts.Force, "force", "f", false, "overwrite existing skills without prompting") + cmd.Flags().StringVar(&opts.Pin, "pin", "", "Pin to a specific git tag or commit SHA") + cmd.Flags().StringVar(&opts.Dir, "dir", "", "Install to a custom directory (overrides --agent and --scope)") + cmd.Flags().BoolVarP(&opts.Force, "force", "f", false, "Overwrite existing skills without prompting") + cmd.Flags().BoolVar(&opts.FromLocal, "from-local", false, "Treat the argument as a local directory path instead of a repository") return cmd } -func installRun(opts *installOptions) error { +func installRun(opts *InstallOptions) error { cs := opts.IO.ColorScheme() canPrompt := opts.IO.CanPrompt() @@ -278,6 +266,8 @@ func installRun(opts *installOptions) error { } } + printPreInstallDisclaimer(opts.IO.ErrOut, cs) + selectedHosts, err := resolveHosts(opts, canPrompt) if err != nil { return err @@ -325,7 +315,7 @@ func installRun(opts *installOptions) error { cs.SuccessIcon(), name, repoSource, discovery.ShortRef(resolved.Ref), friendlyDir(result.Dir)) } - printFileTree(opts.IO.Out, cs, result.Dir, result.Installed) + printFileTree(opts.IO.ErrOut, cs, result.Dir, result.Installed) printReviewHint(opts.IO.ErrOut, cs, repoSource, resolved.SHA, result.Installed) } @@ -337,33 +327,8 @@ func installRun(opts *installOptions) error { return nil } -// isLocalPath returns true if the argument looks like a local filesystem path -// rather than a GitHub owner/repo reference. -func isLocalPath(arg string) bool { - if arg == "" { - return false - } - sep := string(filepath.Separator) - if arg == "." || arg == ".." || - strings.HasPrefix(arg, "./") || strings.HasPrefix(arg, "../") || - strings.HasPrefix(arg, "."+sep) || strings.HasPrefix(arg, ".."+sep) { - return true - } - // filepath.IsAbs on Windows requires a drive letter, so "/tmp/foo" - // would not be recognized. Check explicitly for a leading "/" so that - // Unix-style absolute paths are never mistaken for owner/repo refs. - if filepath.IsAbs(arg) || arg[0] == '/' || strings.HasPrefix(arg, "~") { - return true - } - info, err := os.Stat(arg) - if err == nil && info.IsDir() { - return true - } - return false -} - // runLocalInstall handles installation from a local directory path. -func runLocalInstall(opts *installOptions) error { +func runLocalInstall(opts *InstallOptions) error { cs := opts.IO.ColorScheme() canPrompt := opts.IO.CanPrompt() sourcePath := opts.localPath @@ -401,6 +366,8 @@ func runLocalInstall(opts *installOptions) error { return err } + printPreInstallDisclaimer(opts.IO.ErrOut, cs) + selectedHosts, err := resolveHosts(opts, canPrompt) if err != nil { return err @@ -438,7 +405,7 @@ func runLocalInstall(opts *installOptions) error { name, opts.SkillSource, friendlyDir(result.Dir)) } - printFileTree(opts.IO.Out, cs, result.Dir, result.Installed) + printFileTree(opts.IO.ErrOut, cs, result.Dir, result.Installed) printReviewHint(opts.IO.ErrOut, cs, "", "", result.Installed) } @@ -481,7 +448,7 @@ func resolveRepoArg(skillSource string, canPrompt bool, p prompter.Prompter) (gh return repo, skillSource, nil } -func parseSkillFromOpts(opts *installOptions) { +func parseSkillFromOpts(opts *InstallOptions) { if opts.SkillName != "" { if name, version, ok := cutLast(opts.SkillName, "@"); ok && name != "" { opts.version = version @@ -503,7 +470,7 @@ func cutLast(s, sep string) (before, after string, found bool) { return s, "", false } -func resolveVersion(opts *installOptions, client *api.Client, 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() @@ -514,11 +481,17 @@ func resolveVersion(opts *installOptions, client *api.Client, hostname string) ( return resolved, nil } -func discoverSkills(opts *installOptions, client *api.Client, 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() if err != nil { + var treeTooLarge *discovery.TreeTooLargeError + if errors.As(err, &treeTooLarge) { + fmt.Fprintf(opts.IO.ErrOut, "%s\n Use path-based install instead: gh skill install %s/%s skills/\n", + err, treeTooLarge.Owner, treeTooLarge.Repo) + return nil, err + } return nil, err } logConventions(opts.IO, skills) @@ -527,9 +500,6 @@ func discoverSkills(opts *installOptions, client *api.Client, hostname string, r fmt.Fprintf(opts.IO.ErrOut, "Warning: skill %q does not follow the agentskills.io naming convention\n", s.DisplayName()) } } - sort.Slice(skills, func(i, j int) bool { - return skills[i].DisplayName() < skills[j].DisplayName() - }) return skills, nil } @@ -552,7 +522,7 @@ func logConventions(io *iostreams.IOStreams, skills []discovery.Skill) { // skillSelector holds the callbacks that differ between remote and local skill selection. type skillSelector struct { // matchByName resolves a skill name to matching skills. - matchByName func(opts *installOptions, skills []discovery.Skill) ([]discovery.Skill, error) + matchByName func(opts *InstallOptions, skills []discovery.Skill) ([]discovery.Skill, error) // sourceHint is shown in collision error guidance (e.g. "owner/repo" or "/path/to/skills"). sourceHint string // fetchDescriptions, if non-nil, is called before prompting to pre-populate descriptions. @@ -565,9 +535,13 @@ type installPlan struct { skills []discovery.Skill } -func selectSkillsWithSelector(opts *installOptions, skills []discovery.Skill, canPrompt bool, sel skillSelector) ([]discovery.Skill, error) { +func selectSkillsWithSelector(opts *InstallOptions, skills []discovery.Skill, canPrompt bool, sel skillSelector) ([]discovery.Skill, error) { checkCollisions := func(ss []discovery.Skill) error { - return collisionError(ss, sel.sourceHint) + if err := collisionError(ss); err != nil { + fmt.Fprintf(opts.IO.ErrOut, "Hint: install individually using the full name: gh skill install %s namespace/skill-name\n", sel.sourceHint) + return err + } + return nil } if opts.SkillName != "" { @@ -619,7 +593,7 @@ func selectSkillsWithSelector(opts *installOptions, skills []discovery.Skill, ca return result, checkCollisions(result) } -func matchSkillByName(opts *installOptions, skills []discovery.Skill) ([]discovery.Skill, error) { +func matchSkillByName(opts *InstallOptions, skills []discovery.Skill) ([]discovery.Skill, error) { for _, s := range skills { if s.DisplayName() == opts.SkillName { return []discovery.Skill{s}, nil @@ -644,13 +618,13 @@ func matchSkillByName(opts *installOptions, skills []discovery.Skill) ([]discove names[i] = m.DisplayName() } return nil, fmt.Errorf( - "skill name %q is ambiguous — multiple matches found:\n %s\n Specify the full name (e.g. %s) to disambiguate", + "skill name %q is ambiguous, multiple matches found:\n %s\n Specify the full name (e.g. %s) to disambiguate", opts.SkillName, strings.Join(names, "\n "), names[0], ) } } -func matchLocalSkillByName(opts *installOptions, skills []discovery.Skill) ([]discovery.Skill, error) { +func matchLocalSkillByName(opts *InstallOptions, skills []discovery.Skill) ([]discovery.Skill, error) { for _, s := range skills { if s.DisplayName() == opts.SkillName || s.Name == opts.SkillName { return []discovery.Skill{s}, nil @@ -687,7 +661,7 @@ func skillSearchFunc(skills []discovery.Skill, descWidth int) func(string) promp for i, s := range matched { keys[i] = s.DisplayName() if s.Description != "" { - labels[i] = fmt.Sprintf("%s — %s", s.DisplayName(), truncateDescription(s.Description, descWidth)) + labels[i] = fmt.Sprintf("%s - %s", s.DisplayName(), truncateDescription(s.Description, descWidth)) } else { labels[i] = s.DisplayName() } @@ -720,22 +694,17 @@ func matchSelectedSkills(skills []discovery.Skill, selected []string) ([]discove return result, nil } -// 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 { +// collisionError checks for name collisions among the selected skills. +func collisionError(ss []discovery.Skill) error { collisions := discovery.FindNameCollisions(ss) if len(collisions) == 0 { return nil } - return errors.New(heredoc.Docf(` - cannot install skills with conflicting names — they would overwrite each other: - %s - Install these skills individually using the full name: - gh skill install %s namespace/skill-name - `, discovery.FormatCollisions(collisions), sourceHint)) + return fmt.Errorf("cannot install skills with conflicting names; they would overwrite each other:\n %s", + discovery.FormatCollisions(collisions)) } -func resolveHosts(opts *installOptions, canPrompt bool) ([]*registry.AgentHost, error) { +func resolveHosts(opts *InstallOptions, canPrompt bool) ([]*registry.AgentHost, error) { if opts.Agent != "" { h, err := registry.FindByID(opts.Agent) if err != nil { @@ -745,7 +714,7 @@ func resolveHosts(opts *installOptions, canPrompt bool) ([]*registry.AgentHost, } if !canPrompt { - h, err := registry.FindByID("github-copilot") + h, err := registry.FindByID(registry.DefaultAgentID) if err != nil { return nil, err } @@ -770,7 +739,7 @@ func resolveHosts(opts *installOptions, canPrompt bool) ([]*registry.AgentHost, return selected, nil } -func resolveScope(opts *installOptions, canPrompt bool) (registry.Scope, error) { +func resolveScope(opts *InstallOptions, canPrompt bool) (registry.Scope, error) { if opts.Dir != "" { return registry.Scope(opts.Scope), nil } @@ -795,7 +764,7 @@ func resolveScope(opts *installOptions, canPrompt bool) (registry.Scope, error) return registry.ScopeUser, nil } -func buildInstallPlans(opts *installOptions, selectedSkills []discovery.Skill, selectedHosts []*registry.AgentHost, scope registry.Scope, gitRoot, homeDir string, canPrompt bool) ([]installPlan, error) { +func buildInstallPlans(opts *InstallOptions, selectedSkills []discovery.Skill, selectedHosts []*registry.AgentHost, scope registry.Scope, gitRoot, homeDir string, canPrompt bool) ([]installPlan, error) { byDir := make(map[string]*installPlan) orderedDirs := make([]string, 0, len(selectedHosts)) @@ -832,7 +801,7 @@ func buildInstallPlans(opts *installOptions, selectedSkills []discovery.Skill, s return plans, nil } -func resolveInstallDir(opts *installOptions, host *registry.AgentHost, scope registry.Scope, gitRoot, homeDir string) (string, error) { +func resolveInstallDir(opts *InstallOptions, host *registry.AgentHost, scope registry.Scope, gitRoot, homeDir string) (string, error) { if opts.Dir != "" { return opts.Dir, nil } @@ -851,7 +820,7 @@ func truncateDescription(s string, maxWidth int) string { return text.Truncate(maxWidth, text.RemoveExcessiveWhitespace(s)) } -func checkOverwrite(opts *installOptions, skills []discovery.Skill, targetDir string, canPrompt bool) ([]discovery.Skill, error) { +func checkOverwrite(opts *InstallOptions, skills []discovery.Skill, targetDir string, canPrompt bool) ([]discovery.Skill, error) { var existing, fresh []discovery.Skill for _, s := range skills { dir := filepath.Join(targetDir, filepath.FromSlash(s.InstallName())) @@ -948,8 +917,10 @@ func friendlyDir(dir string) string { return rel } } - if home, err := os.UserHomeDir(); err == nil && (dir == home || strings.HasPrefix(dir, home+string(filepath.Separator))) { - return "~" + dir[len(home):] + if home, err := os.UserHomeDir(); err == nil { + if rel, err := filepath.Rel(home, dir); err == nil && rel != ".." && !strings.HasPrefix(rel, ".."+string(filepath.Separator)) { + return "~/" + rel + } } return dir } @@ -991,6 +962,12 @@ func printTreeDir(w io.Writer, cs *iostreams.ColorScheme, dir, indent string) { } } +// printPreInstallDisclaimer prints a warning that installed skills are unverified +// and should be inspected before use. +func printPreInstallDisclaimer(w io.Writer, cs *iostreams.ColorScheme) { + fmt.Fprintf(w, "\n%s Skills are not verified by GitHub and may contain prompt injections, hidden instructions, or malicious scripts. Always review skill contents before use.\n\n", cs.WarningIcon()) +} + // printReviewHint warns the user to review installed skills and suggests preview commands. // When sha is non-empty the suggested commands include @SHA so the user previews // exactly the version that was installed. diff --git a/pkg/cmd/skills/install/install_test.go b/pkg/cmd/skills/install/install_test.go index b15f5a9b2..481227524 100644 --- a/pkg/cmd/skills/install/install_test.go +++ b/pkg/cmd/skills/install/install_test.go @@ -15,6 +15,7 @@ import ( "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/discovery" "github.com/cli/cli/v2/pkg/cmdutil" "github.com/cli/cli/v2/pkg/httpmock" "github.com/cli/cli/v2/pkg/iostreams" @@ -27,24 +28,24 @@ func TestNewCmdInstall(t *testing.T) { tests := []struct { name string cli string - wantOpts installOptions + wantOpts InstallOptions wantLocalPath bool wantErr bool }{ { name: "repo argument only", cli: "monalisa/skills-repo", - wantOpts: installOptions{SkillSource: "monalisa/skills-repo", Scope: "project"}, + wantOpts: InstallOptions{SkillSource: "monalisa/skills-repo", Scope: "project"}, }, { name: "repo and skill", cli: "monalisa/skills-repo git-commit", - wantOpts: installOptions{SkillSource: "monalisa/skills-repo", SkillName: "git-commit", Scope: "project"}, + wantOpts: InstallOptions{SkillSource: "monalisa/skills-repo", SkillName: "git-commit", Scope: "project"}, }, { name: "all flags", cli: "monalisa/skills-repo git-commit --agent github-copilot --scope user --pin v1.0.0 --force", - wantOpts: installOptions{ + wantOpts: InstallOptions{ SkillSource: "monalisa/skills-repo", SkillName: "git-commit", Agent: "github-copilot", @@ -56,7 +57,7 @@ func TestNewCmdInstall(t *testing.T) { { name: "dir flag", cli: "monalisa/skills-repo git-commit --dir ./custom-skills", - wantOpts: installOptions{SkillSource: "monalisa/skills-repo", SkillName: "git-commit", Dir: "./custom-skills", Scope: "project"}, + wantOpts: InstallOptions{SkillSource: "monalisa/skills-repo", SkillName: "git-commit", Dir: "./custom-skills", Scope: "project"}, }, { name: "too many args", @@ -76,30 +77,45 @@ func TestNewCmdInstall(t *testing.T) { { name: "alias add works", cli: "monalisa/skills-repo git-commit", - wantOpts: installOptions{SkillSource: "monalisa/skills-repo", SkillName: "git-commit", Scope: "project"}, + wantOpts: InstallOptions{SkillSource: "monalisa/skills-repo", SkillName: "git-commit", Scope: "project"}, }, { - name: "dot-slash local path sets localPath", - cli: "./local-dir", - wantOpts: installOptions{SkillSource: "./local-dir", Scope: "project"}, + name: "from-local flag sets localPath", + cli: "--from-local ./local-dir", + wantOpts: InstallOptions{SkillSource: "./local-dir", Scope: "project", FromLocal: true}, wantLocalPath: true, }, { - name: "absolute path sets localPath", - cli: "/absolute/path", - wantOpts: installOptions{SkillSource: "/absolute/path", Scope: "project"}, + name: "from-local with absolute path", + cli: "--from-local /absolute/path", + wantOpts: InstallOptions{SkillSource: "/absolute/path", Scope: "project", FromLocal: true}, wantLocalPath: true, }, { - name: "tilde path sets localPath", - cli: "~/skills", - wantOpts: installOptions{SkillSource: "~/skills", Scope: "project"}, + name: "from-local with tilde path", + cli: "--from-local ~/skills", + wantOpts: InstallOptions{SkillSource: "~/skills", Scope: "project", FromLocal: true}, wantLocalPath: true, }, { name: "owner/repo does not set localPath", cli: "monalisa/skills-repo", - wantOpts: installOptions{SkillSource: "monalisa/skills-repo", Scope: "project"}, + wantOpts: InstallOptions{SkillSource: "monalisa/skills-repo", Scope: "project"}, + }, + { + name: "local-looking path without --from-local treated as repo", + cli: "./local-dir", + wantOpts: InstallOptions{SkillSource: "./local-dir", Scope: "project"}, + }, + { + name: "from-local without argument errors", + cli: "--from-local", + wantErr: true, + }, + { + name: "from-local with --pin is mutually exclusive", + cli: "--from-local ./local-dir --pin v1.0.0", + wantErr: true, }, } for _, tt := range tests { @@ -111,8 +127,8 @@ func TestNewCmdInstall(t *testing.T) { GitClient: &git.Client{}, } - var gotOpts *installOptions - cmd := NewCmdInstall(f, func(opts *installOptions) error { + var gotOpts *InstallOptions + cmd := NewCmdInstall(f, func(opts *InstallOptions) error { gotOpts = opts return nil }) @@ -138,6 +154,7 @@ func TestNewCmdInstall(t *testing.T) { assert.Equal(t, tt.wantOpts.Pin, gotOpts.Pin) assert.Equal(t, tt.wantOpts.Dir, gotOpts.Dir) assert.Equal(t, tt.wantOpts.Force, gotOpts.Force) + assert.Equal(t, tt.wantOpts.FromLocal, gotOpts.FromLocal) if tt.wantLocalPath { assert.NotEmpty(t, gotOpts.localPath, "expected localPath to be set") } else { @@ -152,7 +169,7 @@ func TestNewCmdInstall(t *testing.T) { f := &cmdutil.Factory{IOStreams: ios, Prompter: &prompter.PrompterMock{}, GitClient: &git.Client{}} cmd := NewCmdInstall(f, nil) - assert.Equal(t, "install []", cmd.Use) + assert.Equal(t, "install [] [flags]", cmd.Use) assert.NotEmpty(t, cmd.Short) assert.NotEmpty(t, cmd.Long) assert.NotEmpty(t, cmd.Example) @@ -243,7 +260,7 @@ func TestInstallRun(t *testing.T) { isTTY bool setup func(t *testing.T) stubs func(*httpmock.Registry) - opts func(ios *iostreams.IOStreams, reg *httpmock.Registry) *installOptions + opts func(ios *iostreams.IOStreams, reg *httpmock.Registry) *InstallOptions verify func(t *testing.T) wantErr string wantStdout string @@ -252,9 +269,9 @@ func TestInstallRun(t *testing.T) { { name: "non-interactive without repo errors", isTTY: false, - opts: func(ios *iostreams.IOStreams, _ *httpmock.Registry) *installOptions { + opts: func(ios *iostreams.IOStreams, _ *httpmock.Registry) *InstallOptions { t.Helper() - return &installOptions{ + return &InstallOptions{ IO: ios, GitClient: &git.Client{RepoDir: t.TempDir()}, } @@ -269,9 +286,9 @@ func TestInstallRun(t *testing.T) { stubDiscoverTree(reg, "monalisa", "skills-repo", "abc123", singleSkillTreeJSON("git-commit", "treeSHA", "blobSHA")) }, - opts: func(ios *iostreams.IOStreams, reg *httpmock.Registry) *installOptions { + opts: func(ios *iostreams.IOStreams, reg *httpmock.Registry) *InstallOptions { t.Helper() - return &installOptions{ + return &InstallOptions{ IO: ios, HttpClient: func() (*http.Client, error) { return &http.Client{Transport: reg}, nil }, GitClient: &git.Client{RepoDir: t.TempDir()}, @@ -292,9 +309,9 @@ func TestInstallRun(t *testing.T) { singleSkillTreeJSON("git-commit", "treeSHA", "blobSHA")) stubInstallFiles(reg, "monalisa", "skills-repo", "treeSHA", "blobSHA", gitCommitContent) }, - opts: func(ios *iostreams.IOStreams, reg *httpmock.Registry) *installOptions { + opts: func(ios *iostreams.IOStreams, reg *httpmock.Registry) *InstallOptions { t.Helper() - return &installOptions{ + return &InstallOptions{ IO: ios, HttpClient: func() (*http.Client, error) { return &http.Client{Transport: reg}, nil }, GitClient: &git.Client{RepoDir: t.TempDir()}, @@ -317,9 +334,9 @@ func TestInstallRun(t *testing.T) { singleSkillTreeJSON("git-commit", "treeSHA", "blobSHA")) stubInstallFiles(reg, "monalisa", "skills-repo", "treeSHA", "blobSHA", gitCommitContent) }, - opts: func(ios *iostreams.IOStreams, reg *httpmock.Registry) *installOptions { + opts: func(ios *iostreams.IOStreams, reg *httpmock.Registry) *InstallOptions { t.Helper() - return &installOptions{ + return &InstallOptions{ IO: ios, HttpClient: func() (*http.Client, error) { return &http.Client{Transport: reg}, nil }, GitClient: &git.Client{RepoDir: t.TempDir()}, @@ -342,9 +359,9 @@ func TestInstallRun(t *testing.T) { singleSkillTreeJSON("git-commit", "treeSHA", "blobSHA")) stubInstallFiles(reg, "monalisa", "skills-repo", "treeSHA", "blobSHA", gitCommitContent) }, - opts: func(ios *iostreams.IOStreams, reg *httpmock.Registry) *installOptions { + opts: func(ios *iostreams.IOStreams, reg *httpmock.Registry) *InstallOptions { t.Helper() - return &installOptions{ + return &InstallOptions{ IO: ios, HttpClient: func() (*http.Client, error) { return &http.Client{Transport: reg}, nil }, GitClient: &git.Client{RepoDir: t.TempDir()}, @@ -366,9 +383,9 @@ func TestInstallRun(t *testing.T) { singleSkillTreeJSON("git-commit", "treeSHA", "blobSHA")) stubInstallFiles(reg, "monalisa", "skills-repo", "treeSHA", "blobSHA", gitCommitContent) }, - opts: func(ios *iostreams.IOStreams, reg *httpmock.Registry) *installOptions { + opts: func(ios *iostreams.IOStreams, reg *httpmock.Registry) *InstallOptions { t.Helper() - return &installOptions{ + return &InstallOptions{ IO: ios, HttpClient: func() (*http.Client, error) { return &http.Client{Transport: reg}, nil }, GitClient: &git.Client{RepoDir: t.TempDir()}, @@ -390,9 +407,9 @@ func TestInstallRun(t *testing.T) { singleSkillTreeJSON("git-commit", "treeSHA", "blobSHA")) stubInstallFiles(reg, "monalisa", "skills-repo", "treeSHA", "blobSHA", gitCommitContent) }, - opts: func(ios *iostreams.IOStreams, reg *httpmock.Registry) *installOptions { + opts: func(ios *iostreams.IOStreams, reg *httpmock.Registry) *InstallOptions { t.Helper() - return &installOptions{ + return &InstallOptions{ IO: ios, HttpClient: func() (*http.Client, error) { return &http.Client{Transport: reg}, nil }, GitClient: &git.Client{RepoDir: t.TempDir()}, @@ -413,11 +430,11 @@ func TestInstallRun(t *testing.T) { singleSkillTreeJSON("git-commit", "treeSHA", "blobSHA")) stubInstallFiles(reg, "monalisa", "skills-repo", "treeSHA", "blobSHA", gitCommitContent) }, - opts: func(ios *iostreams.IOStreams, reg *httpmock.Registry) *installOptions { + opts: func(ios *iostreams.IOStreams, reg *httpmock.Registry) *InstallOptions { t.Helper() targetDir := t.TempDir() require.NoError(t, os.MkdirAll(filepath.Join(targetDir, "git-commit"), 0o755)) - return &installOptions{ + return &InstallOptions{ IO: ios, HttpClient: func() (*http.Client, error) { return &http.Client{Transport: reg}, nil }, GitClient: &git.Client{RepoDir: t.TempDir()}, @@ -440,11 +457,11 @@ func TestInstallRun(t *testing.T) { stubDiscoverTree(reg, "monalisa", "skills-repo", "abc123", singleSkillTreeJSON("git-commit", "treeSHA", "blobSHA")) }, - opts: func(ios *iostreams.IOStreams, reg *httpmock.Registry) *installOptions { + opts: func(ios *iostreams.IOStreams, reg *httpmock.Registry) *InstallOptions { t.Helper() targetDir := t.TempDir() require.NoError(t, os.MkdirAll(filepath.Join(targetDir, "git-commit"), 0o755)) - return &installOptions{ + return &InstallOptions{ IO: ios, HttpClient: func() (*http.Client, error) { return &http.Client{Transport: reg}, nil }, GitClient: &git.Client{RepoDir: t.TempDir()}, @@ -466,9 +483,9 @@ func TestInstallRun(t *testing.T) { stubDiscoverTree(reg, "monalisa", "skills-repo", "abc123", singleSkillTreeJSON("git-commit", "treeSHA", "blobSHA")) }, - opts: func(ios *iostreams.IOStreams, reg *httpmock.Registry) *installOptions { + opts: func(ios *iostreams.IOStreams, reg *httpmock.Registry) *InstallOptions { t.Helper() - return &installOptions{ + return &InstallOptions{ IO: ios, HttpClient: func() (*http.Client, error) { return &http.Client{Transport: reg}, nil }, GitClient: &git.Client{RepoDir: t.TempDir()}, @@ -496,9 +513,9 @@ func TestInstallRun(t *testing.T) { `{"path": "skills/bob/xlsx-pro/SKILL.md", "type": "blob", "sha": "blobB"}` stubDiscoverTree(reg, "monalisa", "skills-repo", "abc123", treeJSON) }, - opts: func(ios *iostreams.IOStreams, reg *httpmock.Registry) *installOptions { + opts: func(ios *iostreams.IOStreams, reg *httpmock.Registry) *InstallOptions { t.Helper() - return &installOptions{ + return &InstallOptions{ IO: ios, HttpClient: func() (*http.Client, error) { return &http.Client{Transport: reg}, nil }, GitClient: &git.Client{RepoDir: t.TempDir()}, @@ -527,9 +544,9 @@ func TestInstallRun(t *testing.T) { stubInstallFiles(reg, "monalisa", "skills-repo", "treeB", "blobB", "---\nname: xlsx-pro\ndescription: Bob version\n---\n# B\n") }, - opts: func(ios *iostreams.IOStreams, reg *httpmock.Registry) *installOptions { + opts: func(ios *iostreams.IOStreams, reg *httpmock.Registry) *InstallOptions { t.Helper() - return &installOptions{ + return &InstallOptions{ IO: ios, HttpClient: func() (*http.Client, error) { return &http.Client{Transport: reg}, nil }, GitClient: &git.Client{RepoDir: t.TempDir()}, @@ -546,9 +563,9 @@ func TestInstallRun(t *testing.T) { { name: "remote install with invalid repo argument errors", isTTY: false, - opts: func(ios *iostreams.IOStreams, _ *httpmock.Registry) *installOptions { + opts: func(ios *iostreams.IOStreams, _ *httpmock.Registry) *InstallOptions { t.Helper() - return &installOptions{ + return &InstallOptions{ IO: ios, GitClient: &git.Client{RepoDir: t.TempDir()}, SkillSource: "invalid", @@ -572,9 +589,9 @@ func TestInstallRun(t *testing.T) { singleSkillTreeJSON("git-commit", "treeSHA", "blobSHA")) stubInstallFiles(reg, "monalisa", "skills-repo", "treeSHA", "blobSHA", gitCommitContent) }, - opts: func(ios *iostreams.IOStreams, reg *httpmock.Registry) *installOptions { + opts: func(ios *iostreams.IOStreams, reg *httpmock.Registry) *InstallOptions { t.Helper() - return &installOptions{ + return &InstallOptions{ IO: ios, HttpClient: func() (*http.Client, error) { return &http.Client{Transport: reg}, nil }, GitClient: &git.Client{RepoDir: t.TempDir()}, @@ -590,6 +607,32 @@ func TestInstallRun(t *testing.T) { wantStdout: "Installed git-commit", wantStderr: "v2.0.0", }, + { + name: "remote install shows pre-install disclaimer", + isTTY: true, + stubs: func(reg *httpmock.Registry) { + stubResolveVersion(reg, "monalisa", "skills-repo", "v1.0.0", "abc123") + stubDiscoverTree(reg, "monalisa", "skills-repo", "abc123", + singleSkillTreeJSON("git-commit", "treeSHA", "blobSHA")) + stubInstallFiles(reg, "monalisa", "skills-repo", "treeSHA", "blobSHA", gitCommitContent) + }, + opts: func(ios *iostreams.IOStreams, reg *httpmock.Registry) *InstallOptions { + t.Helper() + return &InstallOptions{ + IO: ios, + HttpClient: func() (*http.Client, error) { return &http.Client{Transport: reg}, nil }, + GitClient: &git.Client{RepoDir: t.TempDir()}, + SkillSource: "monalisa/skills-repo", + SkillName: "git-commit", + Agent: "github-copilot", + Scope: "project", + ScopeChanged: true, + Dir: t.TempDir(), + } + }, + wantStdout: "Installed git-commit", + wantStderr: "not verified by GitHub", + }, { name: "remote install outputs review hint", isTTY: true, @@ -599,9 +642,9 @@ func TestInstallRun(t *testing.T) { singleSkillTreeJSON("git-commit", "treeSHA", "blobSHA")) stubInstallFiles(reg, "monalisa", "skills-repo", "treeSHA", "blobSHA", gitCommitContent) }, - opts: func(ios *iostreams.IOStreams, reg *httpmock.Registry) *installOptions { + opts: func(ios *iostreams.IOStreams, reg *httpmock.Registry) *InstallOptions { t.Helper() - return &installOptions{ + return &InstallOptions{ IO: ios, HttpClient: func() (*http.Client, error) { return &http.Client{Transport: reg}, nil }, GitClient: &git.Client{RepoDir: t.TempDir()}, @@ -625,9 +668,9 @@ func TestInstallRun(t *testing.T) { singleSkillTreeJSON("git-commit", "treeSHA", "blobSHA")) stubInstallFiles(reg, "monalisa", "skills-repo", "treeSHA", "blobSHA", gitCommitContent) }, - opts: func(ios *iostreams.IOStreams, reg *httpmock.Registry) *installOptions { + opts: func(ios *iostreams.IOStreams, reg *httpmock.Registry) *InstallOptions { t.Helper() - return &installOptions{ + return &InstallOptions{ IO: ios, HttpClient: func() (*http.Client, error) { return &http.Client{Transport: reg}, nil }, GitClient: &git.Client{RepoDir: t.TempDir()}, @@ -639,7 +682,7 @@ func TestInstallRun(t *testing.T) { Dir: t.TempDir(), } }, - wantStdout: "SKILL.md", + wantStderr: "SKILL.md", }, { name: "remote install with inline version parses name and version", @@ -656,9 +699,9 @@ func TestInstallRun(t *testing.T) { singleSkillTreeJSON("git-commit", "treeSHA", "blobSHA")) stubInstallFiles(reg, "monalisa", "skills-repo", "treeSHA", "blobSHA", gitCommitContent) }, - opts: func(ios *iostreams.IOStreams, reg *httpmock.Registry) *installOptions { + opts: func(ios *iostreams.IOStreams, reg *httpmock.Registry) *InstallOptions { t.Helper() - return &installOptions{ + return &InstallOptions{ IO: ios, HttpClient: func() (*http.Client, error) { return &http.Client{Transport: reg}, nil }, GitClient: &git.Client{RepoDir: t.TempDir()}, @@ -684,9 +727,9 @@ func TestInstallRun(t *testing.T) { // installer.Install: tree + blob (again, for writing files) stubInstallFiles(reg, "monalisa", "skills-repo", "treeSHA", "blobSHA", gitCommitContent) }, - opts: func(ios *iostreams.IOStreams, reg *httpmock.Registry) *installOptions { + opts: func(ios *iostreams.IOStreams, reg *httpmock.Registry) *InstallOptions { t.Helper() - return &installOptions{ + return &InstallOptions{ IO: ios, HttpClient: func() (*http.Client, error) { return &http.Client{Transport: reg}, nil }, GitClient: &git.Client{RepoDir: t.TempDir()}, @@ -709,9 +752,9 @@ func TestInstallRun(t *testing.T) { singleSkillTreeJSON("git-commit", "treeSHA", "blobSHA")) stubInstallFiles(reg, "monalisa", "skills-repo", "treeSHA", "blobSHA", gitCommitContent) }, - opts: func(ios *iostreams.IOStreams, reg *httpmock.Registry) *installOptions { + opts: func(ios *iostreams.IOStreams, reg *httpmock.Registry) *InstallOptions { t.Helper() - return &installOptions{ + return &InstallOptions{ IO: ios, HttpClient: func() (*http.Client, error) { return &http.Client{Transport: reg}, nil }, GitClient: &git.Client{RepoDir: t.TempDir()}, @@ -737,14 +780,14 @@ func TestInstallRun(t *testing.T) { `{"path": "xlsx-pro/SKILL.md", "type": "blob", "sha": "blob1"}` stubDiscoverTree(reg, "monalisa", "skills-repo", "abc123", treeJSON) }, - opts: func(ios *iostreams.IOStreams, reg *httpmock.Registry) *installOptions { + opts: func(ios *iostreams.IOStreams, reg *httpmock.Registry) *InstallOptions { t.Helper() pm := &prompter.PrompterMock{ MultiSelectWithSearchFunc: func(_, _ string, _, _ []string, _ func(string) prompter.MultiSelectSearchResult) ([]string, error) { return []string{allSkillsKey}, nil }, } - return &installOptions{ + return &InstallOptions{ IO: ios, HttpClient: func() (*http.Client, error) { return &http.Client{Transport: reg}, nil }, Prompter: pm, @@ -784,14 +827,14 @@ func TestInstallRun(t *testing.T) { stubInstallFiles(reg, "monalisa", "skills-repo", "treeB", "blobB", "---\nname: xlsx-pro\ndescription: Bob\n---\n# B\n") }, - opts: func(ios *iostreams.IOStreams, reg *httpmock.Registry) *installOptions { + opts: func(ios *iostreams.IOStreams, reg *httpmock.Registry) *InstallOptions { t.Helper() pm := &prompter.PrompterMock{ MultiSelectWithSearchFunc: func(_, _ string, _, _ []string, _ func(string) prompter.MultiSelectSearchResult) ([]string, error) { return []string{allSkillsKey}, nil }, } - return &installOptions{ + return &InstallOptions{ IO: ios, HttpClient: func() (*http.Client, error) { return &http.Client{Transport: reg}, nil }, Prompter: pm, @@ -814,9 +857,9 @@ func TestInstallRun(t *testing.T) { singleSkillTreeJSON("git-commit", "treeSHA", "blobSHA")) stubInstallFiles(reg, "monalisa", "skills-repo", "treeSHA", "blobSHA", gitCommitContent) }, - opts: func(ios *iostreams.IOStreams, reg *httpmock.Registry) *installOptions { + opts: func(ios *iostreams.IOStreams, reg *httpmock.Registry) *InstallOptions { t.Helper() - return &installOptions{ + return &InstallOptions{ IO: ios, HttpClient: func() (*http.Client, error) { return &http.Client{Transport: reg}, nil }, GitClient: &git.Client{RepoDir: t.TempDir()}, @@ -864,7 +907,7 @@ func TestInstallRun(t *testing.T) { stubInstallFiles(reg, "monalisa", "octocat-skills", "tree-skill-01", "blob-skill-01", "---\nname: skill-01\ndescription: Does skill-01 things\n---\n# Skill\n") }, - opts: func(ios *iostreams.IOStreams, reg *httpmock.Registry) *installOptions { + opts: func(ios *iostreams.IOStreams, reg *httpmock.Registry) *InstallOptions { t.Helper() pm := &prompter.PrompterMock{ MultiSelectWithSearchFunc: func(prompt, searchPrompt string, defaults, persistentOptions []string, searchFunc func(string) prompter.MultiSelectSearchResult) ([]string, error) { @@ -884,7 +927,7 @@ func TestInstallRun(t *testing.T) { return 0, nil }, } - return &installOptions{ + return &InstallOptions{ IO: ios, HttpClient: func() (*http.Client, error) { return &http.Client{Transport: reg}, nil }, Prompter: pm, @@ -905,14 +948,14 @@ func TestInstallRun(t *testing.T) { singleSkillTreeJSON("git-commit", "tree-gc", "blob-gc")) stubInstallFiles(reg, "monalisa", "octocat-skills", "tree-gc", "blob-gc", gitCommitContent) }, - opts: func(ios *iostreams.IOStreams, reg *httpmock.Registry) *installOptions { + opts: func(ios *iostreams.IOStreams, reg *httpmock.Registry) *InstallOptions { t.Helper() pm := &prompter.PrompterMock{ SelectFunc: func(prompt, defaultValue string, options []string) (int, error) { return 0, nil }, } - return &installOptions{ + return &InstallOptions{ IO: ios, HttpClient: func() (*http.Client, error) { return &http.Client{Transport: reg}, nil }, Prompter: pm, @@ -933,7 +976,7 @@ func TestInstallRun(t *testing.T) { stubDiscoverTree(reg, "monalisa", "octocat-skills", "abc123", singleSkillTreeJSON("git-commit", "tree-gc", "blob-gc")) }, - opts: func(ios *iostreams.IOStreams, reg *httpmock.Registry) *installOptions { + opts: func(ios *iostreams.IOStreams, reg *httpmock.Registry) *InstallOptions { t.Helper() destDir := t.TempDir() writeLocalTestSkill(t, destDir, "git-commit", gitCommitContent) @@ -942,7 +985,7 @@ func TestInstallRun(t *testing.T) { return false, nil }, } - return &installOptions{ + return &InstallOptions{ IO: ios, HttpClient: func() (*http.Client, error) { return &http.Client{Transport: reg}, nil }, Prompter: pm, @@ -966,9 +1009,9 @@ func TestInstallRun(t *testing.T) { singleSkillTreeJSON("git-commit", "tree-gc", "blob-gc")) stubInstallFiles(reg, "monalisa", "octocat-skills", "tree-gc", "blob-gc", gitCommitContent) }, - opts: func(ios *iostreams.IOStreams, reg *httpmock.Registry) *installOptions { + opts: func(ios *iostreams.IOStreams, reg *httpmock.Registry) *InstallOptions { t.Helper() - return &installOptions{ + return &InstallOptions{ IO: ios, HttpClient: func() (*http.Client, error) { return &http.Client{Transport: reg}, nil }, GitClient: &git.Client{RepoDir: t.TempDir()}, @@ -996,14 +1039,14 @@ func TestInstallRun(t *testing.T) { singleSkillTreeJSON("git-commit", "tree-gc", "blob-gc")) stubInstallFiles(reg, "monalisa", "octocat-skills", "tree-gc", "blob-gc", gitCommitContent) }, - opts: func(ios *iostreams.IOStreams, reg *httpmock.Registry) *installOptions { + opts: func(ios *iostreams.IOStreams, reg *httpmock.Registry) *InstallOptions { t.Helper() pm := &prompter.PrompterMock{ SelectFunc: func(prompt, defaultValue string, options []string) (int, error) { return 0, nil // project scope }, } - return &installOptions{ + return &InstallOptions{ IO: ios, HttpClient: func() (*http.Client, error) { return &http.Client{Transport: reg}, nil }, Prompter: pm, @@ -1030,7 +1073,7 @@ func TestInstallRun(t *testing.T) { singleSkillTreeJSON("git-commit", "tree-gc", "blob-gc")) stubInstallFiles(reg, "monalisa", "octocat-skills", "tree-gc", "blob-gc", gitCommitContent) }, - opts: func(ios *iostreams.IOStreams, reg *httpmock.Registry) *installOptions { + opts: func(ios *iostreams.IOStreams, reg *httpmock.Registry) *InstallOptions { t.Helper() destDir := t.TempDir() existingContent := heredoc.Doc(` @@ -1050,7 +1093,7 @@ func TestInstallRun(t *testing.T) { return true, nil }, } - return &installOptions{ + return &InstallOptions{ IO: ios, HttpClient: func() (*http.Client, error) { return &http.Client{Transport: reg}, nil }, Prompter: pm, @@ -1068,9 +1111,9 @@ func TestInstallRun(t *testing.T) { { name: "unsupported host returns error", stubs: func(reg *httpmock.Registry) {}, - opts: func(ios *iostreams.IOStreams, reg *httpmock.Registry) *installOptions { + opts: func(ios *iostreams.IOStreams, reg *httpmock.Registry) *InstallOptions { t.Helper() - return &installOptions{ + return &InstallOptions{ IO: ios, HttpClient: func() (*http.Client, error) { return &http.Client{Transport: reg}, nil }, Prompter: &prompter.PrompterMock{}, @@ -1095,7 +1138,7 @@ func TestInstallRun(t *testing.T) { httpmock.StringResponse(fmt.Sprintf(`{"sha": "blob-gc", "content": %q, "encoding": "base64"}`, encoded))) stubInstallFiles(reg, "monalisa", "octocat-skills", "tree-gc", "blob-gc", gitCommitContent) }, - opts: func(ios *iostreams.IOStreams, reg *httpmock.Registry) *installOptions { + opts: func(ios *iostreams.IOStreams, reg *httpmock.Registry) *InstallOptions { t.Helper() pm := &prompter.PrompterMock{ MultiSelectWithSearchFunc: func(prompt, searchPrompt string, defaults, persistentOptions []string, searchFunc func(string) prompter.MultiSelectSearchResult) ([]string, error) { @@ -1105,7 +1148,7 @@ func TestInstallRun(t *testing.T) { return 0, nil }, } - return &installOptions{ + return &InstallOptions{ IO: ios, HttpClient: func() (*http.Client, error) { return &http.Client{Transport: reg}, nil }, Prompter: pm, @@ -1126,7 +1169,7 @@ func TestInstallRun(t *testing.T) { singleSkillTreeJSON("git-commit", "tree-gc", "blob-gc")) stubInstallFiles(reg, "monalisa", "octocat-skills", "tree-gc", "blob-gc", gitCommitContent) }, - opts: func(ios *iostreams.IOStreams, reg *httpmock.Registry) *installOptions { + opts: func(ios *iostreams.IOStreams, reg *httpmock.Registry) *InstallOptions { t.Helper() pm := &prompter.PrompterMock{ InputFunc: func(prompt, defaultValue string) (string, error) { @@ -1136,7 +1179,7 @@ func TestInstallRun(t *testing.T) { return 0, nil }, } - return &installOptions{ + return &InstallOptions{ IO: ios, HttpClient: func() (*http.Client, error) { return &http.Client{Transport: reg}, nil }, Prompter: pm, @@ -1157,14 +1200,14 @@ func TestInstallRun(t *testing.T) { singleSkillTreeJSON("git-commit", "tree-gc", "blob-gc")) stubInstallFiles(reg, "monalisa", "octocat-skills", "tree-gc", "blob-gc", gitCommitContent) }, - opts: func(ios *iostreams.IOStreams, reg *httpmock.Registry) *installOptions { + opts: func(ios *iostreams.IOStreams, reg *httpmock.Registry) *InstallOptions { t.Helper() pm := &prompter.PrompterMock{ SelectFunc: func(prompt, defaultValue string, options []string) (int, error) { return 1, nil // user scope }, } - return &installOptions{ + return &InstallOptions{ IO: ios, HttpClient: func() (*http.Client, error) { return &http.Client{Transport: reg}, nil }, Prompter: pm, @@ -1186,7 +1229,7 @@ func TestInstallRun(t *testing.T) { singleSkillTreeJSON("git-commit", "tree-gc", "blob-gc")) stubInstallFiles(reg, "monalisa", "octocat-skills", "tree-gc", "blob-gc", gitCommitContent) }, - opts: func(ios *iostreams.IOStreams, reg *httpmock.Registry) *installOptions { + opts: func(ios *iostreams.IOStreams, reg *httpmock.Registry) *InstallOptions { t.Helper() destDir := t.TempDir() // Existing skill without github metadata in frontmatter @@ -1204,7 +1247,7 @@ func TestInstallRun(t *testing.T) { return true, nil }, } - return &installOptions{ + return &InstallOptions{ IO: ios, HttpClient: func() (*http.Client, error) { return &http.Client{Transport: reg}, nil }, Prompter: pm, @@ -1232,9 +1275,9 @@ func TestInstallRun(t *testing.T) { stubDiscoverTree(reg, "monalisa", "skills-repo", "abc123", treeJSON) stubInstallFiles(reg, "monalisa", "skills-repo", "treeGC", "blobGC", gitCommitContent) }, - opts: func(ios *iostreams.IOStreams, reg *httpmock.Registry) *installOptions { + opts: func(ios *iostreams.IOStreams, reg *httpmock.Registry) *InstallOptions { t.Helper() - return &installOptions{ + return &InstallOptions{ IO: ios, HttpClient: func() (*http.Client, error) { return &http.Client{Transport: reg}, nil }, GitClient: &git.Client{RepoDir: t.TempDir()}, @@ -1259,7 +1302,7 @@ func TestInstallRun(t *testing.T) { stubInstallFiles(reg, "monalisa", "octocat-skills", "tree-gc", "blob-gc", gitCommitContent) stubInstallFiles(reg, "monalisa", "octocat-skills", "tree-gc", "blob-gc", gitCommitContent) }, - opts: func(ios *iostreams.IOStreams, reg *httpmock.Registry) *installOptions { + opts: func(ios *iostreams.IOStreams, reg *httpmock.Registry) *InstallOptions { t.Helper() pm := &prompter.PrompterMock{ MultiSelectFunc: func(prompt string, defaults []string, options []string) ([]int, error) { @@ -1269,7 +1312,7 @@ func TestInstallRun(t *testing.T) { return 0, nil // project scope }, } - return &installOptions{ + return &InstallOptions{ IO: ios, HttpClient: func() (*http.Client, error) { return &http.Client{Transport: reg}, nil }, Prompter: pm, @@ -1363,7 +1406,7 @@ func TestInstallRun_DeduplicatesSharedProjectDirAcrossHosts(t *testing.T) { }, } - err := installRun(&installOptions{ + err := installRun(&InstallOptions{ IO: ios, HttpClient: func() (*http.Client, error) { return &http.Client{Transport: reg}, nil }, Prompter: pm, @@ -1382,7 +1425,7 @@ func TestRunLocalInstall(t *testing.T) { name string isTTY bool setup func(t *testing.T, sourceDir, targetDir string) - opts func(ios *iostreams.IOStreams, sourceDir, targetDir string) *installOptions + opts func(ios *iostreams.IOStreams, sourceDir, targetDir string) *InstallOptions verify func(t *testing.T, targetDir string) wantErr string wantStdout string @@ -1401,9 +1444,9 @@ func TestRunLocalInstall(t *testing.T) { # Git Commit `)) }, - opts: func(ios *iostreams.IOStreams, sourceDir, targetDir string) *installOptions { + opts: func(ios *iostreams.IOStreams, sourceDir, targetDir string) *InstallOptions { t.Helper() - return &installOptions{ + return &InstallOptions{ IO: ios, SkillSource: sourceDir, localPath: sourceDir, @@ -1438,9 +1481,9 @@ func TestRunLocalInstall(t *testing.T) { `) require.NoError(t, os.WriteFile(filepath.Join(sourceDir, "SKILL.md"), []byte(content), 0o644)) }, - opts: func(ios *iostreams.IOStreams, sourceDir, targetDir string) *installOptions { + opts: func(ios *iostreams.IOStreams, sourceDir, targetDir string) *InstallOptions { t.Helper() - return &installOptions{ + return &InstallOptions{ IO: ios, SkillSource: sourceDir, localPath: sourceDir, @@ -1465,14 +1508,14 @@ func TestRunLocalInstall(t *testing.T) { fmt.Sprintf("---\nname: xlsx-pro\ndescription: %s xlsx-pro\n---\n# Test\n", ns)) } }, - opts: func(ios *iostreams.IOStreams, sourceDir, targetDir string) *installOptions { + opts: func(ios *iostreams.IOStreams, sourceDir, targetDir string) *InstallOptions { t.Helper() pm := &prompter.PrompterMock{ MultiSelectWithSearchFunc: func(_, _ string, _, _ []string, _ func(string) prompter.MultiSelectSearchResult) ([]string, error) { return []string{allSkillsKey}, nil }, } - return &installOptions{ + return &InstallOptions{ IO: ios, SkillSource: sourceDir, localPath: sourceDir, @@ -1505,14 +1548,14 @@ func TestRunLocalInstall(t *testing.T) { } require.NoError(t, os.MkdirAll(filepath.Join(targetDir, "alice", "xlsx-pro"), 0o755)) }, - opts: func(ios *iostreams.IOStreams, sourceDir, targetDir string) *installOptions { + opts: func(ios *iostreams.IOStreams, sourceDir, targetDir string) *InstallOptions { t.Helper() pm := &prompter.PrompterMock{ MultiSelectWithSearchFunc: func(_, _ string, _, _ []string, _ func(string) prompter.MultiSelectSearchResult) ([]string, error) { return []string{allSkillsKey}, nil }, } - return &installOptions{ + return &InstallOptions{ IO: ios, SkillSource: sourceDir, localPath: sourceDir, @@ -1541,9 +1584,9 @@ func TestRunLocalInstall(t *testing.T) { `)) require.NoError(t, os.MkdirAll(filepath.Join(targetDir, "git-commit"), 0o755)) }, - opts: func(ios *iostreams.IOStreams, sourceDir, targetDir string) *installOptions { + opts: func(ios *iostreams.IOStreams, sourceDir, targetDir string) *InstallOptions { t.Helper() - return &installOptions{ + return &InstallOptions{ IO: ios, SkillSource: sourceDir, localPath: sourceDir, @@ -1561,9 +1604,9 @@ func TestRunLocalInstall(t *testing.T) { name: "local install with no skills found errors", isTTY: false, setup: func(_ *testing.T, _, _ string) {}, - opts: func(ios *iostreams.IOStreams, sourceDir, targetDir string) *installOptions { + opts: func(ios *iostreams.IOStreams, sourceDir, targetDir string) *InstallOptions { t.Helper() - return &installOptions{ + return &InstallOptions{ IO: ios, SkillSource: sourceDir, localPath: sourceDir, @@ -1590,9 +1633,9 @@ func TestRunLocalInstall(t *testing.T) { # Git Commit `)) }, - opts: func(ios *iostreams.IOStreams, sourceDir, targetDir string) *installOptions { + opts: func(ios *iostreams.IOStreams, sourceDir, targetDir string) *InstallOptions { t.Helper() - return &installOptions{ + return &InstallOptions{ IO: ios, SkillSource: sourceDir, localPath: sourceDir, @@ -1620,9 +1663,9 @@ func TestRunLocalInstall(t *testing.T) { # Git Commit `)) }, - opts: func(ios *iostreams.IOStreams, sourceDir, targetDir string) *installOptions { + opts: func(ios *iostreams.IOStreams, sourceDir, targetDir string) *InstallOptions { t.Helper() - return &installOptions{ + return &InstallOptions{ IO: ios, SkillSource: sourceDir, localPath: sourceDir, @@ -1657,9 +1700,9 @@ func TestRunLocalInstall(t *testing.T) { # Code Review `)) }, - opts: func(ios *iostreams.IOStreams, sourceDir, targetDir string) *installOptions { + opts: func(ios *iostreams.IOStreams, sourceDir, targetDir string) *InstallOptions { t.Helper() - return &installOptions{ + return &InstallOptions{ IO: ios, SkillSource: sourceDir, localPath: sourceDir, @@ -1686,9 +1729,9 @@ func TestRunLocalInstall(t *testing.T) { require.NoError(t, os.WriteFile(filepath.Join(skillDir, "scripts", "run.sh"), []byte("#!/bin/bash"), 0o644)) }, - opts: func(ios *iostreams.IOStreams, sourceDir, targetDir string) *installOptions { + opts: func(ios *iostreams.IOStreams, sourceDir, targetDir string) *InstallOptions { t.Helper() - return &installOptions{ + return &InstallOptions{ IO: ios, SkillSource: sourceDir, localPath: sourceDir, @@ -1701,7 +1744,7 @@ func TestRunLocalInstall(t *testing.T) { GitClient: &git.Client{RepoDir: t.TempDir()}, } }, - wantStdout: "SKILL.md", + wantStderr: "SKILL.md", }, { name: "local path with tilde expansion", @@ -1716,11 +1759,11 @@ func TestRunLocalInstall(t *testing.T) { # Git Commit `)) }, - opts: func(ios *iostreams.IOStreams, sourceDir, targetDir string) *installOptions { + opts: func(ios *iostreams.IOStreams, sourceDir, targetDir string) *InstallOptions { t.Helper() t.Setenv("HOME", sourceDir) t.Setenv("USERPROFILE", sourceDir) - return &installOptions{ + return &InstallOptions{ IO: ios, SkillSource: "~/", localPath: "~/", @@ -1748,11 +1791,11 @@ func TestRunLocalInstall(t *testing.T) { # Git Commit `)) }, - opts: func(ios *iostreams.IOStreams, sourceDir, targetDir string) *installOptions { + opts: func(ios *iostreams.IOStreams, sourceDir, targetDir string) *InstallOptions { t.Helper() t.Setenv("HOME", sourceDir) t.Setenv("USERPROFILE", sourceDir) - return &installOptions{ + return &InstallOptions{ IO: ios, SkillSource: "~", localPath: "~", @@ -1780,9 +1823,9 @@ func TestRunLocalInstall(t *testing.T) { # Git Commit `)) }, - opts: func(ios *iostreams.IOStreams, sourceDir, targetDir string) *installOptions { + opts: func(ios *iostreams.IOStreams, sourceDir, targetDir string) *InstallOptions { t.Helper() - return &installOptions{ + return &InstallOptions{ IO: ios, SkillSource: sourceDir, localPath: sourceDir, @@ -1898,3 +1941,42 @@ func Test_printReviewHint(t *testing.T) { }) } } + +func Test_printPreInstallDisclaimer(t *testing.T) { + ios, _, _, _ := iostreams.Test() + cs := ios.ColorScheme() + var buf strings.Builder + printPreInstallDisclaimer(&buf, cs) + output := buf.String() + assert.Contains(t, output, "not verified by GitHub") + assert.Contains(t, output, "prompt") + assert.Contains(t, output, "malicious") +} + +func Test_selectSkillsWithSelector_noDisclaimer(t *testing.T) { + ios, _, _, stderr := iostreams.Test() + ios.SetStdoutTTY(true) + ios.SetStderrTTY(true) + + skills := []discovery.Skill{ + {Name: "git-commit", Convention: "skills", Path: "skills/git-commit/SKILL.md"}, + } + + pm := &prompter.PrompterMock{ + MultiSelectWithSearchFunc: func(_, _ string, _, _ []string, _ func(string) prompter.MultiSelectSearchResult) ([]string, error) { + return []string{"git-commit"}, nil + }, + } + + opts := &InstallOptions{ + IO: ios, + Prompter: pm, + } + + _, err := selectSkillsWithSelector(opts, skills, true, skillSelector{ + matchByName: matchSkillByName, + sourceHint: "owner/repo", + }) + require.NoError(t, err) + assert.NotContains(t, stderr.String(), "not verified by GitHub") +} diff --git a/pkg/cmd/skills/install/install_windows_test.go b/pkg/cmd/skills/install/install_windows_test.go deleted file mode 100644 index 8a184fac4..000000000 --- a/pkg/cmd/skills/install/install_windows_test.go +++ /dev/null @@ -1,63 +0,0 @@ -//go:build windows - -package install - -import ( - "os" - "path/filepath" - "testing" - - "github.com/stretchr/testify/assert" -) - -func TestIsLocalPath_Windows(t *testing.T) { - tests := []struct { - name string - arg string - want bool - }{ - // Backslash-relative paths that only exist on Windows. - {`dot-backslash prefix`, `.\skills`, true}, - {`dotdot-backslash prefix`, `..\other`, true}, - {`drive-absolute path`, `C:\Users\me\skills`, true}, - {`drive-relative path`, `D:\projects`, true}, - {`UNC path`, `\\server\share\skills`, true}, - - // Forward-slash forms should still work on Windows. - {`dot-slash prefix`, `./skills`, true}, - {`dotdot-slash prefix`, `../other`, true}, - {`current dir`, `.`, true}, - {`absolute unix-style`, `/tmp/skills`, true}, - {`tilde prefix`, `~/skills`, true}, - - // owner/repo should never be treated as local. - {`owner-repo`, `github/awesome-copilot`, false}, - {`simple name`, `awesome-copilot`, false}, - {`empty string`, ``, false}, - } - - for _, tt := range tests { - t.Run(tt.name, func(t *testing.T) { - got := isLocalPath(tt.arg) - assert.Equal(t, tt.want, got, "isLocalPath(%q)", tt.arg) - }) - } -} - -func TestIsLocalPath_WindowsExistingDir(t *testing.T) { - // A directory that exists on disk should be detected as local even when - // its name looks like owner/repo (the os.Stat safety-net). - dir := t.TempDir() - nested := filepath.Join(dir, "owner", "repo") - if err := os.MkdirAll(nested, 0o755); err != nil { - t.Fatal(err) - } - - // Use a relative path that happens to contain a backslash separator. - rel, err := filepath.Rel(".", nested) - if err != nil { - // If we can't compute a relative path, just use the absolute one. - rel = nested - } - assert.True(t, isLocalPath(rel), "existing dir should be detected as local: %s", rel) -} diff --git a/pkg/cmd/skills/preview/preview.go b/pkg/cmd/skills/preview/preview.go index ee33b04c1..270912478 100644 --- a/pkg/cmd/skills/preview/preview.go +++ b/pkg/cmd/skills/preview/preview.go @@ -21,7 +21,7 @@ import ( "github.com/spf13/cobra" ) -type previewOptions struct { +type PreviewOptions struct { IO *iostreams.IOStreams HttpClient func() (*http.Client, error) Prompter prompter.Prompter @@ -36,8 +36,8 @@ type previewOptions struct { } // NewCmdPreview creates the "skills preview" command. -func NewCmdPreview(f *cmdutil.Factory, runF func(*previewOptions) error) *cobra.Command { - opts := &previewOptions{ +func NewCmdPreview(f *cmdutil.Factory, runF func(*PreviewOptions) error) *cobra.Command { + opts := &PreviewOptions{ IO: f.IOStreams, HttpClient: f.HttpClient, Prompter: f.Prompter, @@ -49,7 +49,7 @@ func NewCmdPreview(f *cmdutil.Factory, runF func(*previewOptions) error) *cobra. cmd := &cobra.Command{ Use: "preview []", - Short: "Preview a skill from a GitHub repository", + Short: "Preview a skill from a GitHub repository (preview)", 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 @@ -109,7 +109,7 @@ func NewCmdPreview(f *cmdutil.Factory, runF func(*previewOptions) error) *cobra. return cmd } -func previewRun(opts *previewOptions) error { +func previewRun(opts *PreviewOptions) error { cs := opts.IO.ColorScheme() repo := opts.repo @@ -186,7 +186,7 @@ func previewRun(opts *previewOptions) error { } // renderAllFiles dumps the tree, SKILL.md, and all extra files through the pager. -func renderAllFiles(opts *previewOptions, cs *iostreams.ColorScheme, skill discovery.Skill, +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 { @@ -213,11 +213,11 @@ func renderAllFiles(opts *previewOptions, cs *iostreams.ColorScheme, skill disco totalBytes := 0 for _, f := range extraFiles { if fetched >= maxFiles { - fmt.Fprintf(out, "\n%s\n", cs.Muted(fmt.Sprintf("(skipped remaining files — showing first %d)", maxFiles))) + fmt.Fprintf(out, "\n%s\n", cs.Muted(fmt.Sprintf("(skipped remaining files, showing first %d)", maxFiles))) break } if totalBytes+f.Size > maxTotalBytes && fetched > 0 { - fmt.Fprintf(out, "\n%s\n", cs.Muted("(skipped remaining files — size limit reached)")) + fmt.Fprintf(out, "\n%s\n", cs.Muted("(skipped remaining files, size limit reached)")) break } fileContent, fetchErr := discovery.FetchBlob(apiClient, hostname, owner, repo, f.SHA) @@ -238,7 +238,7 @@ func renderAllFiles(opts *previewOptions, cs *iostreams.ColorScheme, skill disco } // renderInteractive shows the file tree, then a picker to browse individual files. -func renderInteractive(opts *previewOptions, cs *iostreams.ColorScheme, skill discovery.Skill, +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 { @@ -254,7 +254,7 @@ func renderInteractive(opts *previewOptions, cs *iostreams.ColorScheme, skill di choices = append(choices, f.Path) } - // Save original stdout — StopPager closes IO.Out, so we need to + // Save original stdout. StopPager closes IO.Out, so we need to // restore a working writer before each StartPager call. originalOut := opts.IO.Out @@ -276,7 +276,7 @@ func renderInteractive(opts *previewOptions, cs *iostreams.ColorScheme, skill di } else { selectedFile := extraFiles[idx-1] - // Fetch on demand — don't hold blob data in memory + // 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) @@ -296,7 +296,7 @@ func renderInteractive(opts *previewOptions, cs *iostreams.ColorScheme, skill di } } -func (opts *previewOptions) renderFile(filePath, content string) string { +func (opts *PreviewOptions) renderFile(filePath, content string) string { if opts.RenderFile != nil { return opts.RenderFile(filePath, content) } @@ -304,7 +304,7 @@ func (opts *previewOptions) renderFile(filePath, content string) string { return renderMarkdownPreview(opts.IO, filePath, content) } -func renderSelectedFilePreview(opts *previewOptions, filePath, content string) string { +func renderSelectedFilePreview(opts *PreviewOptions, filePath, content string) string { if !isMarkdownFile(filePath) { return content } @@ -340,7 +340,7 @@ func isMarkdownFile(filePath string) bool { } } -func selectSkill(opts *previewOptions, skills []discovery.Skill) (discovery.Skill, error) { +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 { diff --git a/pkg/cmd/skills/preview/preview_test.go b/pkg/cmd/skills/preview/preview_test.go index debdfbff2..474ce88b5 100644 --- a/pkg/cmd/skills/preview/preview_test.go +++ b/pkg/cmd/skills/preview/preview_test.go @@ -73,8 +73,8 @@ func TestNewCmdPreview(t *testing.T) { Prompter: &prompter.PrompterMock{}, } - var gotOpts *previewOptions - cmd := NewCmdPreview(f, func(opts *previewOptions) error { + var gotOpts *PreviewOptions + cmd := NewCmdPreview(f, func(opts *PreviewOptions) error { gotOpts = opts return nil }) @@ -112,7 +112,7 @@ func TestPreviewRun(t *testing.T) { tests := []struct { name string - opts *previewOptions + opts *PreviewOptions tty bool httpStubs func(*httpmock.Registry) wantStdout string @@ -121,7 +121,7 @@ func TestPreviewRun(t *testing.T) { { name: "preview specific skill", tty: true, - opts: &previewOptions{ + opts: &PreviewOptions{ repo: ghrepo.New("github", "awesome-copilot"), SkillName: "my-skill", }, @@ -164,7 +164,7 @@ func TestPreviewRun(t *testing.T) { { name: "preview with display name match", tty: true, - opts: &previewOptions{ + opts: &PreviewOptions{ repo: ghrepo.New("owner", "repo"), SkillName: "ns/my-skill", }, @@ -208,7 +208,7 @@ func TestPreviewRun(t *testing.T) { { name: "skill not found", tty: true, - opts: &previewOptions{ + opts: &PreviewOptions{ repo: ghrepo.New("owner", "repo"), SkillName: "nonexistent", }, @@ -238,7 +238,7 @@ func TestPreviewRun(t *testing.T) { { name: "no skill name non-interactive errors", tty: false, - opts: &previewOptions{ + opts: &PreviewOptions{ repo: ghrepo.New("owner", "repo"), }, httpStubs: func(reg *httpmock.Registry) { @@ -267,7 +267,7 @@ func TestPreviewRun(t *testing.T) { { name: "preview with explicit version", tty: true, - opts: &previewOptions{ + opts: &PreviewOptions{ repo: ghrepo.New("github", "awesome-copilot"), SkillName: "my-skill", Version: "abc123def456", @@ -350,7 +350,7 @@ func TestPreviewRun(t *testing.T) { func TestPreviewRun_UnsupportedHost(t *testing.T) { ios, _, _, _ := iostreams.Test() - err := previewRun(&previewOptions{ + err := previewRun(&PreviewOptions{ IO: ios, HttpClient: func() (*http.Client, error) { return &http.Client{}, nil }, repo: ghrepo.NewWithHost("github", "awesome-copilot", "acme.ghes.com"), @@ -410,7 +410,7 @@ func TestPreviewRun_Interactive(t *testing.T) { }, } - opts := &previewOptions{ + opts := &PreviewOptions{ IO: ios, HttpClient: func() (*http.Client, error) { return &http.Client{Transport: reg}, nil }, Prompter: pm, @@ -504,7 +504,7 @@ func TestPreviewRun_ShowsFileTree(t *testing.T) { }, } - opts := &previewOptions{ + opts := &PreviewOptions{ IO: ios, HttpClient: func() (*http.Client, error) { return &http.Client{Transport: reg}, nil }, Prompter: pm, @@ -583,7 +583,7 @@ func TestPreviewRun_ShowsFileTree(t *testing.T) { }, } - opts := &previewOptions{ + opts := &PreviewOptions{ IO: ios, HttpClient: func() (*http.Client, error) { return &http.Client{Transport: reg}, nil }, Prompter: pm, @@ -612,7 +612,7 @@ func TestPreviewRun_ShowsFileTree(t *testing.T) { ios.SetStdinTTY(false) ios.SetColorEnabled(false) - opts := &previewOptions{ + opts := &PreviewOptions{ IO: ios, HttpClient: func() (*http.Client, error) { return &http.Client{Transport: reg}, nil }, Prompter: &prompter.PrompterMock{}, @@ -718,7 +718,7 @@ func TestPreviewRun_RenderLimits(t *testing.T) { ios.SetStdoutTTY(false) ios.SetStdinTTY(false) - opts := &previewOptions{ + opts := &PreviewOptions{ IO: ios, HttpClient: func() (*http.Client, error) { return &http.Client{Transport: reg}, nil }, Prompter: &prompter.PrompterMock{}, @@ -749,13 +749,13 @@ func TestPreviewRun_RenderLimits(t *testing.T) { httpmock.REST("GET", "repos/monalisa/skills-repo/git/blobs/blob000"), httpmock.StringResponse(fmt.Sprintf(`{"sha": "blob000", "content": "%s", "encoding": "base64"}`, bigContent)), ) - // blob001 should NOT be fetched — size limit reached + // blob001 should NOT be fetched (size limit reached) ios, _, stdout, _ := iostreams.Test() ios.SetStdoutTTY(false) ios.SetStdinTTY(false) - opts := &previewOptions{ + opts := &PreviewOptions{ IO: ios, HttpClient: func() (*http.Client, error) { return &http.Client{Transport: reg}, nil }, Prompter: &prompter.PrompterMock{}, @@ -787,7 +787,7 @@ func TestPreviewRun_RenderLimits(t *testing.T) { ios.SetStdoutTTY(false) ios.SetStdinTTY(false) - opts := &previewOptions{ + opts := &PreviewOptions{ IO: ios, HttpClient: func() (*http.Client, error) { return &http.Client{Transport: reg}, nil }, Prompter: &prompter.PrompterMock{}, diff --git a/pkg/cmd/skills/publish/publish.go b/pkg/cmd/skills/publish/publish.go index 22d87bb73..82202514b 100644 --- a/pkg/cmd/skills/publish/publish.go +++ b/pkg/cmd/skills/publish/publish.go @@ -27,25 +27,21 @@ import ( "github.com/spf13/cobra" ) -// publishOptions holds all dependencies and user-provided flags for the publish command. -type publishOptions struct { +// 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) + Dir string + Fix bool + DryRun bool + Tag string - // Flags - Fix bool // --fix flag: auto-fix issues where possible - 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 + client *api.Client // injectable for tests; nil means use factory + host string // resolved from config in production } // publishDiagnostic is a single validation finding. @@ -90,8 +86,8 @@ type repoSecurityResponse struct { } // NewCmdPublish creates the "skills publish" command. -func NewCmdPublish(f *cmdutil.Factory, runF func(*publishOptions) error) *cobra.Command { - opts := &publishOptions{ +func NewCmdPublish(f *cmdutil.Factory, runF func(*PublishOptions) error) *cobra.Command { + opts := &PublishOptions{ IO: f.IOStreams, HttpClient: f.HttpClient, Config: f.Config, @@ -100,8 +96,8 @@ func NewCmdPublish(f *cmdutil.Factory, runF func(*publishOptions) error) *cobra. } cmd := &cobra.Command{ - Use: "publish []", - Short: "Validate and publish skills to a GitHub repository", + Use: "publish [] [flags]", + Short: "Validate and publish skills to a GitHub repository (preview)", Long: heredoc.Doc(` Validate a local repository's skills against the Agent Skills specification and publish them by creating a GitHub release. @@ -158,7 +154,7 @@ func NewCmdPublish(f *cmdutil.Factory, runF func(*publishOptions) error) *cobra. return cmd } -func publishRun(opts *publishOptions) error { +func publishRun(opts *PublishOptions) error { dir := opts.Dir if dir == "" { var err error @@ -478,7 +474,7 @@ func fetchTags(client *api.Client, host, owner, repo string) []tagEntry { } // 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 { +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() @@ -515,7 +511,7 @@ func runPublishRelease(opts *publishOptions, client *api.Client, host, owner, re if canPrompt { strategies := []string{ - fmt.Sprintf("Semver (recommended) — %s", suggested), + fmt.Sprintf("Semver (recommended): %s", suggested), "Custom tag", } idx, err := opts.Prompter.Select("Tagging strategy:", "", strategies) @@ -550,7 +546,7 @@ func runPublishRelease(opts *publishOptions, client *api.Client, host, owner, re // 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) + return fmt.Errorf("tag %s already exists; choose a different version", tag) } } @@ -565,7 +561,7 @@ func runPublishRelease(opts *publishOptions, client *api.Client, host, owner, re 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") + 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()) } @@ -707,7 +703,7 @@ func checkTagProtection(client *api.Client, host, owner, repo string) []publishD return []publishDiagnostic{{ severity: "warning", - message: "no active tag protection rulesets found — consider protecting tags to ensure immutable releases (Settings → Rules → Rulesets)", + message: "no active tag protection rulesets found. Consider protecting tags to ensure immutable releases (Settings > Rules > Rulesets)", }} } @@ -732,14 +728,14 @@ func checkSecuritySettings(client *api.Client, host, owner, repo, skillsDir stri 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)", + 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)", + message: "secret scanning push protection is not enabled. Blocks pushes containing secrets (gh repo edit --enable-secret-scanning-push-protection)", }) } @@ -750,7 +746,7 @@ func checkSecuritySettings(client *api.Client, host, owner, repo, skillsDir stri 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)", + message: "skills include code files but code scanning does not appear to be configured (Settings > Code security > Code scanning)", }) } } @@ -760,7 +756,7 @@ func checkSecuritySettings(client *api.Client, host, owner, repo, skillsDir stri 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)", + message: "skills include dependency manifests but Dependabot alerts do not appear to be enabled (Settings > Code security > Dependabot)", }) } } @@ -825,7 +821,15 @@ func checkInstalledSkillDirs(gitClient *git.Client, repoDir string) []publishDia if gitClient != nil { ignoreGitClient := gitClient.Copy() ignoreGitClient.RepoDir = repoDir - if ignoreGitClient.IsIgnored(context.Background(), relPath) { + ignored, err := ignoreGitClient.IsIgnored(context.Background(), relPath) + if ignored { + continue + } + if err != nil { + diagnostics = append(diagnostics, publishDiagnostic{ + severity: "warning", + message: fmt.Sprintf("%s/ may contain installed skills that are not gitignored (could not verify: %v)", relPath, err), + }) continue } } @@ -883,7 +887,7 @@ func detectGitHubRemote(gitClient *git.Client) (ghrepo.Interface, error) { // Fall back to any remote that points to GitHub remotes, err := gitClient.Remotes(context.Background()) if err != nil { - return nil, nil + return nil, nil //nolint:nilerr // failing to list remotes is not an error; it just means no repo detected } for _, r := range remotes { if r.Name == "origin" { @@ -907,14 +911,14 @@ func detectGitHubRemote(gitClient *git.Client) (ghrepo.Interface, error) { func parseGitHubURL(rawURL string) (ghrepo.Interface, error) { u, err := git.ParseURL(rawURL) if err != nil { - return nil, nil + return nil, nil //nolint:nilerr // unparseable URL means it's not a GitHub remote } r, err := ghrepo.FromURL(u) if err != nil { - return nil, nil + return nil, nil //nolint:nilerr // URL didn't match GitHub repo format } if err := source.ValidateSupportedHost(r.RepoHost()); err != nil { - return nil, nil + return nil, nil //nolint:nilerr // non-GitHub host is silently ignored } return r, nil } @@ -930,7 +934,7 @@ func detectMissingRepoDiagnostic(gitClient *git.Client, dir string) []publishDia if _, err := dirGitClient.GitDir(context.Background()); err != nil { return []publishDiagnostic{{ severity: "warning", - message: "not a git repository — initialize with: git init && gh repo create", + message: "not a git repository. Initialize with: git init && gh repo create", }} } @@ -938,7 +942,7 @@ func detectMissingRepoDiagnostic(gitClient *git.Client, dir string) []publishDia if err != nil || len(remotes) == 0 { return []publishDiagnostic{{ severity: "warning", - message: "no git remote found — create a GitHub repository with: gh repo create", + message: "no git remote found. Create a GitHub repository with: gh repo create", }} } @@ -950,11 +954,11 @@ func detectMissingRepoDiagnostic(gitClient *git.Client, dir string) []publishDia } 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, ", ")), + 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) { +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 @@ -1016,7 +1020,7 @@ func renderDiagnosticsTTY(opts *publishOptions, skillDirs []string, diagnostics } } -func renderDiagnosticsPlain(opts *publishOptions, diagnostics []publishDiagnostic, errors, warnings int) { +func renderDiagnosticsPlain(opts *PublishOptions, diagnostics []publishDiagnostic, errors, warnings int) { for _, d := range diagnostics { if d.severity == "info" { continue diff --git a/pkg/cmd/skills/publish/publish_test.go b/pkg/cmd/skills/publish/publish_test.go index d54d04ad3..fdcaa6631 100644 --- a/pkg/cmd/skills/publish/publish_test.go +++ b/pkg/cmd/skills/publish/publish_test.go @@ -76,12 +76,12 @@ func TestNewCmdPublish(t *testing.T) { name string cli string wantsErr bool - wantsOpts publishOptions + wantsOpts PublishOptions }{ { name: "all flags", cli: "./monalisa-skills --dry-run --fix --tag v1.0.0", - wantsOpts: publishOptions{ + wantsOpts: PublishOptions{ Dir: "./monalisa-skills", DryRun: true, Fix: true, @@ -91,19 +91,19 @@ func TestNewCmdPublish(t *testing.T) { { name: "directory only", cli: "./octocat-repo", - wantsOpts: publishOptions{ + wantsOpts: PublishOptions{ Dir: "./octocat-repo", }, }, { name: "no args leaves dir empty", cli: "", - wantsOpts: publishOptions{}, + wantsOpts: PublishOptions{}, }, { name: "dry-run flag only", cli: "--dry-run", - wantsOpts: publishOptions{ + wantsOpts: PublishOptions{ DryRun: true, }, }, @@ -113,8 +113,8 @@ func TestNewCmdPublish(t *testing.T) { ios, _, _, _ := iostreams.Test() f := cmdutil.Factory{IOStreams: ios} - var gotOpts *publishOptions - cmd := NewCmdPublish(&f, func(opts *publishOptions) error { + var gotOpts *PublishOptions + cmd := NewCmdPublish(&f, func(opts *PublishOptions) error { gotOpts = opts return nil }) @@ -151,7 +151,7 @@ func TestPublishRun_UnsupportedHost(t *testing.T) { `)) ios, _, _, _ := iostreams.Test() - err := publishRun(&publishOptions{ + err := publishRun(&PublishOptions{ IO: ios, Dir: dir, GitClient: newTestGitClient(t, map[string]string{"origin": "https://github.com/monalisa/skills-repo.git"}), @@ -167,7 +167,7 @@ func TestPublishRun(t *testing.T) { isTTY bool setup func(t *testing.T, dir string) stubs func(*httpmock.Registry) - opts func(ios *iostreams.IOStreams, dir string, reg *httpmock.Registry) *publishOptions + opts func(ios *iostreams.IOStreams, dir string, reg *httpmock.Registry) *PublishOptions verify func(t *testing.T, dir string) wantErr string wantStdout string @@ -176,9 +176,9 @@ func TestPublishRun(t *testing.T) { { name: "no skills directory", setup: func(_ *testing.T, _ string) {}, - opts: func(ios *iostreams.IOStreams, dir string, _ *httpmock.Registry) *publishOptions { + opts: func(ios *iostreams.IOStreams, dir string, _ *httpmock.Registry) *PublishOptions { t.Helper() - return &publishOptions{IO: ios, Dir: dir} + return &PublishOptions{IO: ios, Dir: dir} }, wantErr: "no skills/ directory", }, @@ -188,9 +188,9 @@ func TestPublishRun(t *testing.T) { t.Helper() require.NoError(t, os.MkdirAll(filepath.Join(dir, "skills", "empty-skill"), 0o755)) }, - opts: func(ios *iostreams.IOStreams, dir string, _ *httpmock.Registry) *publishOptions { + opts: func(ios *iostreams.IOStreams, dir string, _ *httpmock.Registry) *PublishOptions { t.Helper() - return &publishOptions{IO: ios, Dir: dir} + return &PublishOptions{IO: ios, Dir: dir} }, wantErr: "validation failed", wantStdout: "missing SKILL.md", @@ -206,9 +206,9 @@ func TestPublishRun(t *testing.T) { Body text. `)) }, - opts: func(ios *iostreams.IOStreams, dir string, _ *httpmock.Registry) *publishOptions { + opts: func(ios *iostreams.IOStreams, dir string, _ *httpmock.Registry) *PublishOptions { t.Helper() - return &publishOptions{IO: ios, Dir: dir} + return &PublishOptions{IO: ios, Dir: dir} }, wantErr: "validation failed", wantStdout: "missing required field: name", @@ -225,9 +225,9 @@ func TestPublishRun(t *testing.T) { Body. `)) }, - opts: func(ios *iostreams.IOStreams, dir string, _ *httpmock.Registry) *publishOptions { + opts: func(ios *iostreams.IOStreams, dir string, _ *httpmock.Registry) *PublishOptions { t.Helper() - return &publishOptions{IO: ios, Dir: dir} + return &PublishOptions{IO: ios, Dir: dir} }, wantErr: "validation failed", wantStdout: "does not match directory name", @@ -244,9 +244,9 @@ func TestPublishRun(t *testing.T) { Body. `)) }, - opts: func(ios *iostreams.IOStreams, dir string, _ *httpmock.Registry) *publishOptions { + opts: func(ios *iostreams.IOStreams, dir string, _ *httpmock.Registry) *PublishOptions { t.Helper() - return &publishOptions{IO: ios, Dir: dir} + return &PublishOptions{IO: ios, Dir: dir} }, wantErr: "validation failed", wantStdout: "naming convention", @@ -268,9 +268,9 @@ func TestPublishRun(t *testing.T) { stubs: func(reg *httpmock.Registry) { stubAllSecureRemote(reg, "monalisa", "skills-repo") }, - opts: func(ios *iostreams.IOStreams, dir string, reg *httpmock.Registry) *publishOptions { + opts: func(ios *iostreams.IOStreams, dir string, reg *httpmock.Registry) *PublishOptions { t.Helper() - return &publishOptions{ + return &PublishOptions{ IO: ios, Dir: dir, DryRun: true, @@ -320,9 +320,9 @@ func TestPublishRun(t *testing.T) { }), ) }, - opts: func(ios *iostreams.IOStreams, dir string, reg *httpmock.Registry) *publishOptions { + opts: func(ios *iostreams.IOStreams, dir string, reg *httpmock.Registry) *PublishOptions { t.Helper() - return &publishOptions{ + return &PublishOptions{ IO: ios, Dir: dir, Tag: "v1.0.1", @@ -356,9 +356,9 @@ func TestPublishRun(t *testing.T) { Body. `)) }, - opts: func(ios *iostreams.IOStreams, dir string, _ *httpmock.Registry) *publishOptions { + opts: func(ios *iostreams.IOStreams, dir string, _ *httpmock.Registry) *PublishOptions { t.Helper() - return &publishOptions{IO: ios, Dir: dir, Fix: true} + return &PublishOptions{IO: ios, Dir: dir, Fix: true} }, wantStdout: "stripped install metadata", verify: func(t *testing.T, dir string) { @@ -386,9 +386,9 @@ func TestPublishRun(t *testing.T) { Body. `)) }, - opts: func(ios *iostreams.IOStreams, dir string, _ *httpmock.Registry) *publishOptions { + opts: func(ios *iostreams.IOStreams, dir string, _ *httpmock.Registry) *PublishOptions { t.Helper() - return &publishOptions{IO: ios, Dir: dir, Fix: false} + return &PublishOptions{IO: ios, Dir: dir, Fix: false} }, wantErr: "validation failed", wantStdout: "--fix", @@ -405,9 +405,9 @@ func TestPublishRun(t *testing.T) { Body. `)) }, - opts: func(ios *iostreams.IOStreams, dir string, _ *httpmock.Registry) *publishOptions { + opts: func(ios *iostreams.IOStreams, dir string, _ *httpmock.Registry) *PublishOptions { t.Helper() - return &publishOptions{IO: ios, Dir: dir} + return &PublishOptions{IO: ios, Dir: dir} }, wantStdout: "license", }, @@ -426,9 +426,9 @@ func TestPublishRun(t *testing.T) { Body. `)) }, - opts: func(ios *iostreams.IOStreams, dir string, _ *httpmock.Registry) *publishOptions { + opts: func(ios *iostreams.IOStreams, dir string, _ *httpmock.Registry) *PublishOptions { t.Helper() - return &publishOptions{IO: ios, Dir: dir} + return &PublishOptions{IO: ios, Dir: dir} }, wantErr: "validation failed", wantStdout: "allowed-tools must be a string", @@ -473,9 +473,9 @@ func TestPublishRun(t *testing.T) { }), ) }, - opts: func(ios *iostreams.IOStreams, dir string, reg *httpmock.Registry) *publishOptions { + opts: func(ios *iostreams.IOStreams, dir string, reg *httpmock.Registry) *PublishOptions { t.Helper() - return &publishOptions{ + return &PublishOptions{ IO: ios, Dir: dir, GitClient: newTestGitClient(t, map[string]string{ @@ -525,9 +525,9 @@ func TestPublishRun(t *testing.T) { }), ) }, - opts: func(ios *iostreams.IOStreams, dir string, reg *httpmock.Registry) *publishOptions { + opts: func(ios *iostreams.IOStreams, dir string, reg *httpmock.Registry) *PublishOptions { t.Helper() - return &publishOptions{ + return &PublishOptions{ IO: ios, Dir: dir, GitClient: newTestGitClient(t, map[string]string{ @@ -587,9 +587,9 @@ func TestPublishRun(t *testing.T) { httpmock.StatusStringResponse(404, "not found"), ) }, - opts: func(ios *iostreams.IOStreams, dir string, reg *httpmock.Registry) *publishOptions { + opts: func(ios *iostreams.IOStreams, dir string, reg *httpmock.Registry) *PublishOptions { t.Helper() - return &publishOptions{ + return &PublishOptions{ IO: ios, Dir: dir, DryRun: true, @@ -651,9 +651,9 @@ func TestPublishRun(t *testing.T) { httpmock.StatusStringResponse(404, "not found"), ) }, - opts: func(ios *iostreams.IOStreams, dir string, reg *httpmock.Registry) *publishOptions { + opts: func(ios *iostreams.IOStreams, dir string, reg *httpmock.Registry) *PublishOptions { t.Helper() - return &publishOptions{ + return &PublishOptions{ IO: ios, Dir: dir, DryRun: true, @@ -680,14 +680,14 @@ func TestPublishRun(t *testing.T) { Body. `)) }, - opts: func(ios *iostreams.IOStreams, dir string, _ *httpmock.Registry) *publishOptions { + opts: func(ios *iostreams.IOStreams, dir string, _ *httpmock.Registry) *PublishOptions { t.Helper() require.NoError(t, os.MkdirAll(filepath.Join(dir, ".agents", "skills", "installed"), 0o755)) runGitInDir(t, dir, "init", "--initial-branch=main") runGitInDir(t, dir, "config", "user.email", "monalisa@github.com") runGitInDir(t, dir, "config", "user.name", "Monalisa Octocat") - return &publishOptions{ + return &PublishOptions{ IO: ios, Dir: dir, GitClient: &git.Client{RepoDir: dir}, @@ -716,9 +716,9 @@ func TestPublishRun(t *testing.T) { runGitInDir(t, dir, "add", ".gitignore") runGitInDir(t, dir, "commit", "-m", "init") }, - opts: func(ios *iostreams.IOStreams, dir string, _ *httpmock.Registry) *publishOptions { + opts: func(ios *iostreams.IOStreams, dir string, _ *httpmock.Registry) *PublishOptions { t.Helper() - return &publishOptions{ + return &PublishOptions{ IO: ios, Dir: dir, GitClient: &git.Client{RepoDir: dir}, @@ -730,6 +730,31 @@ func TestPublishRun(t *testing.T) { // The key assertion: .gitignored dirs should NOT produce a warning }, }, + { + name: "installed skill dirs git error warns about unverified status", + setup: func(t *testing.T, dir string) { + t.Helper() + writeSkill(t, dir, "my-skill", heredoc.Doc(` + --- + name: my-skill + description: A skill + license: MIT + --- + Body. + `)) + // Create install dir but do NOT init git so check-ignore will fail + require.NoError(t, os.MkdirAll(filepath.Join(dir, ".agents", "skills", "installed"), 0o755)) + }, + opts: func(ios *iostreams.IOStreams, dir string, _ *httpmock.Registry) *PublishOptions { + t.Helper() + return &PublishOptions{ + IO: ios, + Dir: dir, + GitClient: &git.Client{RepoDir: dir}, + } + }, + wantStdout: "may contain installed skills that are not gitignored", + }, { name: "no GitHub remote warns", setup: func(t *testing.T, dir string) { @@ -747,9 +772,9 @@ func TestPublishRun(t *testing.T) { runGitInDir(t, dir, "config", "user.name", "Monalisa Octocat") runGitInDir(t, dir, "remote", "add", "origin", "https://gitlab.com/hubot/bar.git") }, - opts: func(ios *iostreams.IOStreams, dir string, _ *httpmock.Registry) *publishOptions { + opts: func(ios *iostreams.IOStreams, dir string, _ *httpmock.Registry) *PublishOptions { t.Helper() - return &publishOptions{ + return &PublishOptions{ IO: ios, Dir: dir, GitClient: &git.Client{RepoDir: dir}, @@ -774,9 +799,9 @@ func TestPublishRun(t *testing.T) { stubs: func(reg *httpmock.Registry) { stubAllSecureRemote(reg, "octocat", "repo") }, - opts: func(ios *iostreams.IOStreams, dir string, reg *httpmock.Registry) *publishOptions { + opts: func(ios *iostreams.IOStreams, dir string, reg *httpmock.Registry) *PublishOptions { t.Helper() - return &publishOptions{ + return &PublishOptions{ IO: ios, Dir: dir, DryRun: true, @@ -860,9 +885,9 @@ func TestPublishRun(t *testing.T) { }), ) }, - opts: func(ios *iostreams.IOStreams, dir string, reg *httpmock.Registry) *publishOptions { + opts: func(ios *iostreams.IOStreams, dir string, reg *httpmock.Registry) *PublishOptions { t.Helper() - return &publishOptions{ + return &PublishOptions{ IO: ios, Dir: dir, Tag: "v1.0.0", @@ -937,9 +962,9 @@ func TestPublishRun(t *testing.T) { }), ) }, - opts: func(ios *iostreams.IOStreams, dir string, reg *httpmock.Registry) *publishOptions { + opts: func(ios *iostreams.IOStreams, dir string, reg *httpmock.Registry) *PublishOptions { t.Helper() - return &publishOptions{ + return &PublishOptions{ IO: ios, Dir: dir, Tag: "v2.3.5", @@ -968,9 +993,9 @@ func TestPublishRun(t *testing.T) { stubs: func(reg *httpmock.Registry) { stubAllSecureRemote(reg, "monalisa", "skills-repo") }, - opts: func(ios *iostreams.IOStreams, dir string, reg *httpmock.Registry) *publishOptions { + opts: func(ios *iostreams.IOStreams, dir string, reg *httpmock.Registry) *PublishOptions { t.Helper() - return &publishOptions{ + return &PublishOptions{ IO: ios, Dir: dir, Tag: "v1.0.0", // same as stubAllSecureRemote's existing tag @@ -1000,9 +1025,9 @@ func TestPublishRun(t *testing.T) { stubs: func(reg *httpmock.Registry) { stubAllSecureRemote(reg, "monalisa", "skills-repo") }, - opts: func(ios *iostreams.IOStreams, dir string, reg *httpmock.Registry) *publishOptions { + opts: func(ios *iostreams.IOStreams, dir string, reg *httpmock.Registry) *PublishOptions { t.Helper() - return &publishOptions{ + return &PublishOptions{ IO: ios, Dir: dir, GitClient: newTestGitClient(t, map[string]string{ @@ -1027,9 +1052,9 @@ func TestPublishRun(t *testing.T) { Body. `)) }, - opts: func(ios *iostreams.IOStreams, dir string, _ *httpmock.Registry) *publishOptions { + opts: func(ios *iostreams.IOStreams, dir string, _ *httpmock.Registry) *PublishOptions { t.Helper() - return &publishOptions{ + return &PublishOptions{ IO: ios, Dir: dir, } @@ -1051,7 +1076,7 @@ func TestPublishRun(t *testing.T) { `)) }, stubs: func(reg *httpmock.Registry) { - // No topic yet — first GET for diagnostic check + // No topic yet, first GET for diagnostic check reg.Register( httpmock.REST("GET", "repos/monalisa/skills-repo/topics"), httpmock.JSONResponse(map[string]interface{}{"names": []string{}}), @@ -1097,10 +1122,10 @@ func TestPublishRun(t *testing.T) { }), ) }, - opts: func(ios *iostreams.IOStreams, dir string, reg *httpmock.Registry) *publishOptions { + opts: func(ios *iostreams.IOStreams, dir string, reg *httpmock.Registry) *PublishOptions { t.Helper() confirmCall := 0 - return &publishOptions{ + return &PublishOptions{ IO: ios, Dir: dir, Prompter: &prompter.PrompterMock{ @@ -1155,9 +1180,9 @@ func TestPublishRun(t *testing.T) { }), ) }, - opts: func(ios *iostreams.IOStreams, dir string, reg *httpmock.Registry) *publishOptions { + opts: func(ios *iostreams.IOStreams, dir string, reg *httpmock.Registry) *PublishOptions { t.Helper() - return &publishOptions{ + return &PublishOptions{ IO: ios, Dir: dir, Prompter: &prompter.PrompterMock{ @@ -1205,10 +1230,10 @@ func TestPublishRun(t *testing.T) { httpmock.JSONResponse(map[string]interface{}{"default_branch": "main"}), ) }, - opts: func(ios *iostreams.IOStreams, dir string, reg *httpmock.Registry) *publishOptions { + opts: func(ios *iostreams.IOStreams, dir string, reg *httpmock.Registry) *PublishOptions { t.Helper() confirmCall := 0 - return &publishOptions{ + return &PublishOptions{ IO: ios, Dir: dir, Prompter: &prompter.PrompterMock{ @@ -1273,9 +1298,9 @@ func TestPublishRun(t *testing.T) { }), ) }, - opts: func(ios *iostreams.IOStreams, dir string, reg *httpmock.Registry) *publishOptions { + opts: func(ios *iostreams.IOStreams, dir string, reg *httpmock.Registry) *PublishOptions { t.Helper() - return &publishOptions{ + return &PublishOptions{ IO: ios, Dir: dir, Prompter: &prompter.PrompterMock{ diff --git a/pkg/cmd/skills/search/search.go b/pkg/cmd/skills/search/search.go index 48ea9f358..abf925275 100644 --- a/pkg/cmd/skills/search/search.go +++ b/pkg/cmd/skills/search/search.go @@ -45,7 +45,7 @@ var SkillSearchFields = []string{ "path", } -type searchOptions struct { +type SearchOptions struct { IO *iostreams.IOStreams HttpClient func() (*http.Client, error) Config func() (gh.Config, error) @@ -61,8 +61,8 @@ type searchOptions struct { } // NewCmdSearch creates the "skills search" command. -func NewCmdSearch(f *cmdutil.Factory, runF func(*searchOptions) error) *cobra.Command { - opts := &searchOptions{ +func NewCmdSearch(f *cmdutil.Factory, runF func(*SearchOptions) error) *cobra.Command { + opts := &SearchOptions{ IO: f.IOStreams, HttpClient: f.HttpClient, Config: f.Config, @@ -71,8 +71,8 @@ func NewCmdSearch(f *cmdutil.Factory, runF func(*searchOptions) error) *cobra.Co } cmd := &cobra.Command{ - Use: "search ", - Short: "Search for skills across GitHub", + Use: "search [flags]", + Short: "Search for skills across GitHub (preview)", Long: heredoc.Doc(` Search across all public GitHub repositories for skills matching a keyword. @@ -188,7 +188,7 @@ func (s skillResult) ExportData(fields []string) map[string]interface{} { return data } -func searchRun(opts *searchOptions) error { +func searchRun(opts *SearchOptions) error { httpClient, err := opts.HttpClient() if err != nil { return err @@ -252,7 +252,7 @@ func searchRun(opts *searchOptions) error { } // noResultsMessage returns an appropriate "no results" message. -func noResultsMessage(opts *searchOptions) string { +func noResultsMessage(opts *SearchOptions) string { if opts.Owner != "" { return fmt.Sprintf("no skills found matching %q from owner %q", opts.Query, opts.Owner) } @@ -328,7 +328,7 @@ func searchByKeyword(client *api.Client, host, queryTerm, owner string, page, li return nil, primaryErr } - // Merge: path-matched → hyphen-matched → owner-matched → primary content. + // Merge: path-matched > hyphen-matched > owner-matched > primary content. var merged []codeSearchItem if pathErr == nil && pathResult != nil { @@ -346,7 +346,7 @@ func searchByKeyword(client *api.Client, host, queryTerm, owner string, page, li } // noResults returns an empty JSON array for exporters or a no-results error. -func noResults(opts *searchOptions, msg string) error { +func noResults(opts *SearchOptions, msg string) error { if opts.Exporter != nil { return opts.Exporter.Write(opts.IO, []skillResult{}) } @@ -433,7 +433,7 @@ func deduplicateByName(skills []skillResult) []skillResult { } // renderResults handles all output modes: JSON, interactive picker, or table. -func renderResults(opts *searchOptions, skills []skillResult, totalPages int) error { +func renderResults(opts *SearchOptions, skills []skillResult, totalPages int) error { if opts.Exporter != nil { return opts.Exporter.Write(opts.IO, skills) } @@ -499,7 +499,7 @@ func renderTable(io *iostreams.IOStreams, skills []skillResult) error { // 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 { +func promptInstall(opts *SearchOptions, skills []skillResult) error { fmt.Fprintln(opts.IO.ErrOut) cs := opts.IO.ColorScheme() @@ -589,7 +589,7 @@ func relevanceScore(s skillResult, query string) int { score := 0 // Name match. Normalize spaces to hyphens since skill directory names - // use hyphens as word separators (e.g. query "mcp apps" → "mcp-apps"). + // use hyphens as word separators (e.g. query "mcp apps" > "mcp-apps"). skillLower := strings.ToLower(s.SkillName) if skillLower == term || skillLower == termHyphen { score += 3_000 @@ -825,7 +825,7 @@ func extractSkillName(filePath string) string { return discovery.MatchesSkillPath(filePath) } -// formatStars formats a star count for display (e.g. 1700 → "1.7k"). +// formatStars formats a star count for display (e.g. 1700 > "1.7k"). // TODO kw: Could be swaped for go-humanize. func formatStars(n int) string { if n >= 1000 { diff --git a/pkg/cmd/skills/search/search_test.go b/pkg/cmd/skills/search/search_test.go index 6e35465cd..98d26d146 100644 --- a/pkg/cmd/skills/search/search_test.go +++ b/pkg/cmd/skills/search/search_test.go @@ -23,7 +23,7 @@ func TestSearchRun_UnsupportedHost(t *testing.T) { cfg.AuthenticationFunc = func() gh.AuthConfig { return authCfg } - err := searchRun(&searchOptions{ + err := searchRun(&SearchOptions{ IO: ios, Query: "terraform", Page: 1, @@ -38,33 +38,33 @@ func TestNewCmdSearch(t *testing.T) { tests := []struct { name string args string - wantOpts searchOptions + wantOpts SearchOptions wantErr string }{ { name: "query argument", args: "terraform", - wantOpts: searchOptions{Query: "terraform", Page: 1, Limit: defaultLimit}, + wantOpts: SearchOptions{Query: "terraform", Page: 1, Limit: defaultLimit}, }, { name: "with page flag", args: "terraform --page 3", - wantOpts: searchOptions{Query: "terraform", Page: 3, Limit: defaultLimit}, + wantOpts: SearchOptions{Query: "terraform", Page: 3, Limit: defaultLimit}, }, { name: "with limit flag", args: "terraform --limit 5", - wantOpts: searchOptions{Query: "terraform", Page: 1, 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}, + 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}, + wantOpts: SearchOptions{Query: "terraform", Owner: "hashicorp", Page: 1, Limit: defaultLimit}, }, { name: "no arguments", @@ -101,8 +101,8 @@ func TestNewCmdSearch(t *testing.T) { 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 { + var gotOpts *SearchOptions + cmd := NewCmdSearch(f, func(opts *SearchOptions) error { gotOpts = opts return nil }) @@ -149,7 +149,7 @@ func TestSearchRun(t *testing.T) { tests := []struct { name string - opts *searchOptions + opts *SearchOptions tty bool httpStubs func(*httpmock.Registry) wantStdout string @@ -159,7 +159,7 @@ func TestSearchRun(t *testing.T) { { name: "displays results in non-TTY", tty: false, - opts: &searchOptions{Query: "terraform", Page: 1, Limit: defaultLimit}, + 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"}}]}`) }, @@ -168,7 +168,7 @@ func TestSearchRun(t *testing.T) { { name: "deduplicates results", tty: false, - opts: &searchOptions{Query: "terraform", Page: 1, Limit: defaultLimit}, + 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"}}]}`) }, @@ -177,7 +177,7 @@ func TestSearchRun(t *testing.T) { { name: "no results", tty: true, - opts: &searchOptions{Query: "nonexistent", Page: 1, Limit: defaultLimit}, + opts: &SearchOptions{Query: "nonexistent", Page: 1, Limit: defaultLimit}, httpStubs: func(reg *httpmock.Registry) { stubKeywordSearch(reg, emptyCodeResponse) }, @@ -186,7 +186,7 @@ func TestSearchRun(t *testing.T) { { name: "nested skill path", tty: false, - opts: &searchOptions{Query: "my-skill", Page: 1, Limit: defaultLimit}, + 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"}}]}`) }, @@ -195,7 +195,7 @@ func TestSearchRun(t *testing.T) { { name: "ranks name-matching results first", tty: false, - opts: &searchOptions{Query: "terraform", Page: 1, Limit: defaultLimit}, + 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"}}, @@ -209,7 +209,7 @@ func TestSearchRun(t *testing.T) { { name: "caps total pages at 1000-result limit", tty: false, - opts: &searchOptions{Query: "terraform", Page: 1, Limit: defaultLimit}, + 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"}}]}`) }, @@ -219,7 +219,7 @@ func TestSearchRun(t *testing.T) { { name: "page beyond available results", tty: false, - opts: &searchOptions{Query: "terraform", Page: 999, Limit: defaultLimit}, + 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"}}]}`) }, @@ -228,10 +228,10 @@ func TestSearchRun(t *testing.T) { { name: "json output with selected fields", tty: false, - opts: func() *searchOptions { + opts: func() *SearchOptions { exporter := cmdutil.NewJSONExporter() exporter.SetFields([]string{"repo", "skillName", "stars"}) - return &searchOptions{Query: "terraform", Page: 1, Limit: defaultLimit, Exporter: exporter} + 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"}}]}`) @@ -241,10 +241,10 @@ func TestSearchRun(t *testing.T) { { name: "json output empty results", tty: false, - opts: func() *searchOptions { + opts: func() *SearchOptions { exporter := cmdutil.NewJSONExporter() exporter.SetFields([]string{"repo", "skillName"}) - return &searchOptions{Query: "nonexistent", Page: 1, Limit: defaultLimit, Exporter: exporter} + return &SearchOptions{Query: "nonexistent", Page: 1, Limit: defaultLimit, Exporter: exporter} }(), httpStubs: func(reg *httpmock.Registry) { stubKeywordSearch(reg, emptyCodeResponse) @@ -254,7 +254,7 @@ func TestSearchRun(t *testing.T) { { name: "rate limit error returns friendly message", tty: false, - opts: &searchOptions{Query: "terraform", Page: 1, Limit: defaultLimit}, + 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 { @@ -272,7 +272,7 @@ func TestSearchRun(t *testing.T) { { name: "HTTP 429 returns rate limit error", tty: false, - opts: &searchOptions{Query: "terraform", Page: 1, Limit: defaultLimit}, + opts: &SearchOptions{Query: "terraform", Page: 1, Limit: defaultLimit}, httpStubs: func(reg *httpmock.Registry) { for range 3 { reg.Register( @@ -286,7 +286,7 @@ func TestSearchRun(t *testing.T) { { name: "HTTP 403 with Retry-After returns rate limit error", tty: false, - opts: &searchOptions{Query: "terraform", Page: 1, Limit: defaultLimit}, + opts: &SearchOptions{Query: "terraform", Page: 1, Limit: defaultLimit}, httpStubs: func(reg *httpmock.Registry) { for range 3 { reg.Register( @@ -303,7 +303,7 @@ func TestSearchRun(t *testing.T) { { name: "no results with owner scope", tty: true, - opts: &searchOptions{Query: "nonexistent", Owner: "monalisa", Page: 1, Limit: defaultLimit}, + opts: &SearchOptions{Query: "nonexistent", Owner: "monalisa", Page: 1, Limit: defaultLimit}, httpStubs: func(reg *httpmock.Registry) { // With --owner set, only path + primary searches fire (no owner search). for range 2 { @@ -318,7 +318,7 @@ func TestSearchRun(t *testing.T) { { name: "enriches results with blob descriptions", tty: false, - opts: &searchOptions{Query: "terraform", Page: 1, Limit: defaultLimit}, + opts: &SearchOptions{Query: "terraform", Page: 1, Limit: defaultLimit}, httpStubs: func(reg *httpmock.Registry) { codeResponse := `{"total_count": 1, "incomplete_results": false, "items": [ {"name": "SKILL.md", "path": "skills/terraform/SKILL.md", "sha": "abc123", diff --git a/pkg/cmd/skills/skills.go b/pkg/cmd/skills/skills.go index 8f9c45faf..2989c52f8 100644 --- a/pkg/cmd/skills/skills.go +++ b/pkg/cmd/skills/skills.go @@ -1,6 +1,7 @@ package skills import ( + "github.com/MakeNowJust/heredoc" "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" @@ -13,11 +14,32 @@ import ( // NewCmdSkills returns the top-level "skill" command. func NewCmdSkills(f *cmdutil.Factory) *cobra.Command { cmd := &cobra.Command{ - Use: "skill ", - Short: "Install and manage agent skills", - Long: "Install and manage agent skills from GitHub repositories.", + Use: "skill ", + Short: "Install and manage agent skills (preview)", + Long: heredoc.Doc(` + Install and manage agent skills from GitHub repositories. + + Working with agent skills in the GitHub CLI is in preview and + subject to change without notice. + `), Aliases: []string{"skills"}, GroupID: "core", + Example: heredoc.Doc(` + # Search for skills + $ gh skill search terraform + + # Install a skill + $ gh skill install github/awesome-copilot code-review + + # Preview a skill before installing + $ gh skill preview github/awesome-copilot code-review + + # Update all installed skills + $ gh skill update --all + + # Validate skills for publishing + $ gh skill publish --dry-run + `), } cmd.AddCommand(install.NewCmdInstall(f, nil)) diff --git a/pkg/cmd/skills/update/update.go b/pkg/cmd/skills/update/update.go index 11db14a34..766f52515 100644 --- a/pkg/cmd/skills/update/update.go +++ b/pkg/cmd/skills/update/update.go @@ -23,23 +23,20 @@ import ( "github.com/spf13/cobra" ) -// updateOptions holds all dependencies and user-provided flags for the update command. -type updateOptions struct { +// 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) - Unpin bool // --unpin flag (clear pinned ref and include in update) - Dir string // --dir flag (scan a custom directory) + Skills []string + All bool + Force bool + DryRun bool + Unpin bool + Dir string } // installedSkill represents a locally installed skill parsed from its SKILL.md frontmatter. @@ -66,8 +63,8 @@ type pendingUpdate struct { } // NewCmdUpdate creates the "skills update" command. -func NewCmdUpdate(f *cmdutil.Factory, runF func(*updateOptions) error) *cobra.Command { - opts := &updateOptions{ +func NewCmdUpdate(f *cmdutil.Factory, runF func(*UpdateOptions) error) *cobra.Command { + opts := &UpdateOptions{ IO: f.IOStreams, Prompter: f.Prompter, Config: f.Config, @@ -76,8 +73,8 @@ func NewCmdUpdate(f *cmdutil.Factory, runF func(*updateOptions) error) *cobra.Co } cmd := &cobra.Command{ - Use: "update [...]", - Short: "Update installed skills to their latest versions", + Use: "update [...] [flags]", + Short: "Update installed skills to their latest versions (preview)", Long: heredoc.Doc(` Checks installed skills for available updates by comparing the local tree SHA (from SKILL.md frontmatter) against the remote repository. @@ -142,7 +139,7 @@ func NewCmdUpdate(f *cmdutil.Factory, runF func(*updateOptions) error) *cobra.Co return cmd } -func updateRun(opts *updateOptions) error { +func updateRun(opts *UpdateOptions) error { cs := opts.IO.ColorScheme() canPrompt := opts.IO.CanPrompt() @@ -190,10 +187,23 @@ func updateRun(opts *updateOptions) error { installed = filtered } - for _, s := range installed { - if s.metadataErr != nil { - return fmt.Errorf("skill %s has invalid repository metadata: %w", s.name, s.metadataErr) + // Skip skills with invalid metadata rather than aborting the entire + // update run. One corrupt skill should not prevent updating others. + { + var valid []installedSkill + for _, s := range installed { + if s.metadataErr != nil { + fmt.Fprintf(opts.IO.ErrOut, "%s Skipping %s: invalid repository metadata: %s\n", cs.WarningIcon(), s.name, s.metadataErr) + continue + } + valid = append(valid, s) } + installed = valid + } + + if len(installed) == 0 { + fmt.Fprintf(opts.IO.ErrOut, "No updatable skills found.\n") + return nil } // Prompt for metadata on skills missing it (before starting progress indicator) @@ -205,7 +215,7 @@ func updateRun(opts *updateOptions) error { name string source string // "owner/repo" } - prompted := make(map[string]promptedEntry) // dir → entry + prompted := make(map[string]promptedEntry) // dir > entry for i := range installed { s := &installed[i] if s.owner != "" && s.repo != "" { @@ -327,7 +337,7 @@ func updateRun(opts *updateOptions) error { fmt.Fprintf(opts.IO.ErrOut, "%s %s is pinned to %s (skipped)\n", cs.Muted("⊘"), 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) + fmt.Fprintf(opts.IO.ErrOut, "%s %s has no GitHub metadata. Reinstall to enable updates\n", cs.WarningIcon(), name) } if len(updates) == 0 { @@ -346,7 +356,7 @@ func updateRun(opts *updateOptions) error { cs.Cyan("•"), u.local.name, u.local.owner, u.local.repo, git.ShortSHA(u.newSHA), discovery.ShortRef(u.resolved.Ref)) } else { - fmt.Fprintf(opts.IO.Out, " %s %s (%s/%s) %s → %s [%s]\n", + 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.Muted(git.ShortSHA(u.local.treeSHA)), git.ShortSHA(u.newSHA), discovery.ShortRef(u.resolved.Ref)) diff --git a/pkg/cmd/skills/update/update_test.go b/pkg/cmd/skills/update/update_test.go index b9aabe86a..0c224e9a0 100644 --- a/pkg/cmd/skills/update/update_test.go +++ b/pkg/cmd/skills/update/update_test.go @@ -30,11 +30,11 @@ func TestNewCmdUpdate_Help(t *testing.T) { GitClient: &git.Client{}, } - cmd := NewCmdUpdate(f, func(opts *updateOptions) error { + cmd := NewCmdUpdate(f, func(opts *UpdateOptions) error { return nil }) - assert.Equal(t, "update [...]", cmd.Use) + assert.Equal(t, "update [...] [flags]", cmd.Use) assert.NotEmpty(t, cmd.Short) assert.NotEmpty(t, cmd.Long) assert.NotEmpty(t, cmd.Example) @@ -43,7 +43,7 @@ func TestNewCmdUpdate_Help(t *testing.T) { 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 }) + cmd := NewCmdUpdate(f, func(_ *UpdateOptions) error { return nil }) flags := []string{"all", "force", "dry-run", "dir", "unpin"} for _, name := range flags { @@ -55,8 +55,8 @@ func TestNewCmdUpdate_ArgsPassedToOptions(t *testing.T) { ios, _, stdout, stderr := iostreams.Test() f := &cmdutil.Factory{IOStreams: ios, Prompter: &prompter.PrompterMock{}, GitClient: &git.Client{}} - var gotOpts *updateOptions - cmd := NewCmdUpdate(f, func(opts *updateOptions) error { + var gotOpts *UpdateOptions + cmd := NewCmdUpdate(f, func(opts *UpdateOptions) error { gotOpts = opts return nil }) @@ -176,7 +176,7 @@ func TestScanInstalledSkills(t *testing.T) { }, { name: "non-existent directory returns nil", - // no setup — dir does not exist + // no setup needed; dir does not exist verify: func(t *testing.T, skills []installedSkill, err error) { t.Helper() require.NoError(t, err) @@ -313,7 +313,7 @@ func TestUpdateRun(t *testing.T) { name string setup func(t *testing.T, dir string) stubs func(reg *httpmock.Registry) - opts func(ios *iostreams.IOStreams, dir string, reg *httpmock.Registry) *updateOptions + opts func(ios *iostreams.IOStreams, dir string, reg *httpmock.Registry) *UpdateOptions verify func(t *testing.T, dir string) wantErr string wantStderr string @@ -349,9 +349,9 @@ func TestUpdateRun(t *testing.T) { httpmock.REST("GET", "repos/monalisa/octocat-skills/git/trees/commit1"), httpmock.StringResponse(`{"sha": "commit1", "tree": [{"path": "skills/code-review", "type": "tree", "sha": "currentsha"}, {"path": "skills/code-review/SKILL.md", "type": "blob", "sha": "blob1"}, {"path": "skills", "type": "tree", "sha": "treeshaX"}], "truncated": false}`)) }, - opts: func(ios *iostreams.IOStreams, dir string, reg *httpmock.Registry) *updateOptions { + opts: func(ios *iostreams.IOStreams, dir string, reg *httpmock.Registry) *UpdateOptions { ios.SetStdoutTTY(false) - return &updateOptions{ + return &UpdateOptions{ IO: ios, Config: func() (gh.Config, error) { return config.NewBlankConfig(), nil }, HttpClient: func() (*http.Client, error) { @@ -365,9 +365,9 @@ func TestUpdateRun(t *testing.T) { { name: "no installed skills", stubs: func(reg *httpmock.Registry) {}, - opts: func(ios *iostreams.IOStreams, dir string, reg *httpmock.Registry) *updateOptions { + opts: func(ios *iostreams.IOStreams, dir string, reg *httpmock.Registry) *UpdateOptions { ios.SetStdoutTTY(false) - return &updateOptions{ + return &UpdateOptions{ IO: ios, Config: func() (gh.Config, error) { return config.NewBlankConfig(), nil }, HttpClient: func() (*http.Client, error) { @@ -395,9 +395,9 @@ func TestUpdateRun(t *testing.T) { `)), 0o644)) }, stubs: func(reg *httpmock.Registry) {}, - opts: func(ios *iostreams.IOStreams, dir string, reg *httpmock.Registry) *updateOptions { + opts: func(ios *iostreams.IOStreams, dir string, reg *httpmock.Registry) *UpdateOptions { ios.SetStdoutTTY(false) - return &updateOptions{ + return &UpdateOptions{ IO: ios, Config: func() (gh.Config, error) { return config.NewBlankConfig(), nil }, HttpClient: func() (*http.Client, error) { @@ -427,10 +427,10 @@ func TestUpdateRun(t *testing.T) { `)), 0o644)) }, stubs: func(reg *httpmock.Registry) {}, - opts: func(ios *iostreams.IOStreams, dir string, reg *httpmock.Registry) *updateOptions { + opts: func(ios *iostreams.IOStreams, dir string, reg *httpmock.Registry) *UpdateOptions { ios.SetStdoutTTY(true) ios.SetStderrTTY(true) - return &updateOptions{ + return &UpdateOptions{ IO: ios, Config: func() (gh.Config, error) { return config.NewBlankConfig(), nil }, HttpClient: func() (*http.Client, error) { @@ -457,10 +457,10 @@ func TestUpdateRun(t *testing.T) { `)), 0o644)) }, stubs: func(reg *httpmock.Registry) {}, - opts: func(ios *iostreams.IOStreams, dir string, reg *httpmock.Registry) *updateOptions { + opts: func(ios *iostreams.IOStreams, dir string, reg *httpmock.Registry) *UpdateOptions { ios.SetStdoutTTY(false) ios.SetStdinTTY(false) - return &updateOptions{ + return &UpdateOptions{ IO: ios, Config: func() (gh.Config, error) { return config.NewBlankConfig(), nil }, HttpClient: func() (*http.Client, error) { @@ -502,9 +502,9 @@ func TestUpdateRun(t *testing.T) { httpmock.StringResponse(fmt.Sprintf(`{"sha": "commitsha123", "tree": [{"path": "skills/monalisa-skill/SKILL.md", "type": "blob", "sha": "blobsha1"}, {"path": "skills/monalisa-skill", "type": "tree", "sha": "abc123def456"}, {"path": "skills", "type": "tree", "sha": "treeshaX"}], "truncated": false}`)), ) }, - opts: func(ios *iostreams.IOStreams, dir string, reg *httpmock.Registry) *updateOptions { + opts: func(ios *iostreams.IOStreams, dir string, reg *httpmock.Registry) *UpdateOptions { ios.SetStdoutTTY(false) - return &updateOptions{ + return &UpdateOptions{ IO: ios, Config: func() (gh.Config, error) { return config.NewBlankConfig(), nil }, HttpClient: func() (*http.Client, error) { @@ -546,10 +546,10 @@ func TestUpdateRun(t *testing.T) { httpmock.StringResponse(`{"sha": "newcommit456", "tree": [{"path": "skills/hubot-skill/SKILL.md", "type": "blob", "sha": "blobsha2"}, {"path": "skills/hubot-skill", "type": "tree", "sha": "newsha456"}, {"path": "skills", "type": "tree", "sha": "treeshaY"}], "truncated": false}`), ) }, - opts: func(ios *iostreams.IOStreams, dir string, reg *httpmock.Registry) *updateOptions { + opts: func(ios *iostreams.IOStreams, dir string, reg *httpmock.Registry) *UpdateOptions { ios.SetStdoutTTY(true) ios.SetStderrTTY(true) - return &updateOptions{ + return &UpdateOptions{ IO: ios, Config: func() (gh.Config, error) { return config.NewBlankConfig(), nil }, HttpClient: func() (*http.Client, error) { @@ -594,10 +594,10 @@ func TestUpdateRun(t *testing.T) { httpmock.StringResponse(`{"sha": "newcommit456", "tree": [{"path": "skills/hubot-skill/SKILL.md", "type": "blob", "sha": "blobsha2"}, {"path": "skills/hubot-skill", "type": "tree", "sha": "newsha456"}, {"path": "skills", "type": "tree", "sha": "treeshaY"}], "truncated": false}`), ) }, - opts: func(ios *iostreams.IOStreams, dir string, reg *httpmock.Registry) *updateOptions { + opts: func(ios *iostreams.IOStreams, dir string, reg *httpmock.Registry) *UpdateOptions { ios.SetStdoutTTY(false) ios.SetStdinTTY(false) - return &updateOptions{ + return &UpdateOptions{ IO: ios, Config: func() (gh.Config, error) { return config.NewBlankConfig(), nil }, HttpClient: func() (*http.Client, error) { @@ -647,9 +647,9 @@ func TestUpdateRun(t *testing.T) { httpmock.StringResponse(fmt.Sprintf(`{"sha": "newblob1", "encoding": "base64", "content": "%s"}`, "IyBDb2RlIFJldmlldyBVcGRhdGVk"))) }, - opts: func(ios *iostreams.IOStreams, dir string, reg *httpmock.Registry) *updateOptions { + opts: func(ios *iostreams.IOStreams, dir string, reg *httpmock.Registry) *UpdateOptions { ios.SetStdoutTTY(false) - return &updateOptions{ + return &UpdateOptions{ IO: ios, Config: func() (gh.Config, error) { return config.NewBlankConfig(), nil }, HttpClient: func() (*http.Client, error) { @@ -708,9 +708,9 @@ func TestUpdateRun(t *testing.T) { httpmock.StringResponse(fmt.Sprintf(`{"sha": "newblob1", "encoding": "base64", "content": "%s"}`, "IyBOYW1lc3BhY2VkIFNraWxsIFVwZGF0ZWQ="))) }, - opts: func(ios *iostreams.IOStreams, dir string, reg *httpmock.Registry) *updateOptions { + opts: func(ios *iostreams.IOStreams, dir string, reg *httpmock.Registry) *UpdateOptions { ios.SetStdoutTTY(false) - return &updateOptions{ + return &UpdateOptions{ IO: ios, Config: func() (gh.Config, error) { return config.NewBlankConfig(), nil }, HttpClient: func() (*http.Client, error) { @@ -768,9 +768,9 @@ func TestUpdateRun(t *testing.T) { httpmock.REST("GET", "repos/monalisa/octocat-skills/git/blobs/newblob1"), httpmock.StatusStringResponse(500, "server error")) }, - opts: func(ios *iostreams.IOStreams, dir string, reg *httpmock.Registry) *updateOptions { + opts: func(ios *iostreams.IOStreams, dir string, reg *httpmock.Registry) *UpdateOptions { ios.SetStdoutTTY(false) - return &updateOptions{ + return &UpdateOptions{ IO: ios, Config: func() (gh.Config, error) { return config.NewBlankConfig(), nil }, HttpClient: func() (*http.Client, error) { @@ -828,11 +828,11 @@ func TestUpdateRun(t *testing.T) { httpmock.StringResponse(fmt.Sprintf(`{"sha": "newblob1", "encoding": "base64", "content": "%s"}`, "IyBDb2RlIFJldmlldyBVcGRhdGVk"))) }, - opts: func(ios *iostreams.IOStreams, dir string, reg *httpmock.Registry) *updateOptions { + opts: func(ios *iostreams.IOStreams, dir string, reg *httpmock.Registry) *UpdateOptions { ios.SetStdoutTTY(true) ios.SetStdinTTY(true) ios.SetStderrTTY(true) - return &updateOptions{ + return &UpdateOptions{ IO: ios, Config: func() (gh.Config, error) { return config.NewBlankConfig(), nil }, HttpClient: func() (*http.Client, error) { @@ -883,11 +883,11 @@ func TestUpdateRun(t *testing.T) { httpmock.REST("GET", "repos/monalisa/octocat-skills/git/trees/newcommit789"), httpmock.StringResponse(`{"sha": "newcommit789", "tree": [{"path": "skills/code-review/SKILL.md", "type": "blob", "sha": "newblob1"}, {"path": "skills/code-review", "type": "tree", "sha": "newsha999"}, {"path": "skills", "type": "tree", "sha": "treeshaZ"}], "truncated": false}`)) }, - opts: func(ios *iostreams.IOStreams, dir string, reg *httpmock.Registry) *updateOptions { + opts: func(ios *iostreams.IOStreams, dir string, reg *httpmock.Registry) *UpdateOptions { ios.SetStdoutTTY(true) ios.SetStdinTTY(true) ios.SetStderrTTY(true) - return &updateOptions{ + return &UpdateOptions{ IO: ios, Config: func() (gh.Config, error) { return config.NewBlankConfig(), nil }, HttpClient: func() (*http.Client, error) { @@ -919,11 +919,11 @@ func TestUpdateRun(t *testing.T) { `)), 0o644)) }, stubs: func(reg *httpmock.Registry) {}, - opts: func(ios *iostreams.IOStreams, dir string, reg *httpmock.Registry) *updateOptions { + opts: func(ios *iostreams.IOStreams, dir string, reg *httpmock.Registry) *UpdateOptions { ios.SetStdoutTTY(true) ios.SetStdinTTY(true) ios.SetStderrTTY(true) - return &updateOptions{ + return &UpdateOptions{ IO: ios, Config: func() (gh.Config, error) { return config.NewBlankConfig(), nil }, HttpClient: func() (*http.Client, error) { @@ -974,11 +974,11 @@ func TestUpdateRun(t *testing.T) { httpmock.StringResponse(fmt.Sprintf(`{"sha": "blob1", "encoding": "base64", "content": "%s"}`, "IyBNYW51YWwgU2tpbGwgVXBkYXRlZA=="))) }, - opts: func(ios *iostreams.IOStreams, dir string, reg *httpmock.Registry) *updateOptions { + opts: func(ios *iostreams.IOStreams, dir string, reg *httpmock.Registry) *UpdateOptions { ios.SetStdoutTTY(true) ios.SetStdinTTY(true) ios.SetStderrTTY(true) - return &updateOptions{ + return &UpdateOptions{ IO: ios, Config: func() (gh.Config, error) { return config.NewBlankConfig(), nil }, HttpClient: func() (*http.Client, error) { @@ -1044,9 +1044,9 @@ func TestUpdateRun(t *testing.T) { httpmock.StringResponse(fmt.Sprintf(`{"sha": "newblob1", "encoding": "base64", "content": "%s"}`, "IyBVbnBpbm5lZCBhbmQgVXBkYXRlZA=="))) }, - opts: func(ios *iostreams.IOStreams, dir string, reg *httpmock.Registry) *updateOptions { + opts: func(ios *iostreams.IOStreams, dir string, reg *httpmock.Registry) *UpdateOptions { ios.SetStdoutTTY(false) - return &updateOptions{ + return &UpdateOptions{ IO: ios, Config: func() (gh.Config, error) { return config.NewBlankConfig(), nil }, HttpClient: func() (*http.Client, error) { @@ -1084,10 +1084,10 @@ func TestUpdateRun(t *testing.T) { `)), 0o644)) }, stubs: func(reg *httpmock.Registry) {}, - opts: func(ios *iostreams.IOStreams, dir string, reg *httpmock.Registry) *updateOptions { + opts: func(ios *iostreams.IOStreams, dir string, reg *httpmock.Registry) *UpdateOptions { ios.SetStdoutTTY(true) ios.SetStderrTTY(true) - return &updateOptions{ + return &UpdateOptions{ IO: ios, Config: func() (gh.Config, error) { return config.NewBlankConfig(), nil }, HttpClient: func() (*http.Client, error) { @@ -1130,10 +1130,10 @@ func TestUpdateRun(t *testing.T) { httpmock.REST("GET", "repos/octocat/hubot-skills/git/trees/newcommit789"), httpmock.StringResponse(`{"sha": "newcommit789", "tree": [{"path": "skills/pinned-skill/SKILL.md", "type": "blob", "sha": "newblob1"}, {"path": "skills/pinned-skill", "type": "tree", "sha": "newsha999"}, {"path": "skills", "type": "tree", "sha": "treeshaZ"}], "truncated": false}`)) }, - opts: func(ios *iostreams.IOStreams, dir string, reg *httpmock.Registry) *updateOptions { + opts: func(ios *iostreams.IOStreams, dir string, reg *httpmock.Registry) *UpdateOptions { ios.SetStdoutTTY(true) ios.SetStderrTTY(true) - return &updateOptions{ + return &UpdateOptions{ IO: ios, Config: func() (gh.Config, error) { return config.NewBlankConfig(), nil }, HttpClient: func() (*http.Client, error) {