From 75909ed2ba1bb034630edf49b18fb58ff5b64c04 Mon Sep 17 00:00:00 2001 From: Kynan Ware <47394200+BagToad@users.noreply.github.com> Date: Wed, 15 Apr 2026 14:09:11 -0600 Subject: [PATCH 01/12] Suggest and install official extensions for unknown commands When a user runs an unknown command that matches a known official extension (e.g. `gh stack` or `gh aw`), show an install suggestion. In interactive TTY sessions, prompt the user to install it on the spot. The official extension registry is hard-coded in pkg/extensions/. Current entries are github/gh-aw and github/gh-stack. Teams can add their extensions via PR. Install suggestions use an explicit github.com host prefix for GHES compatibility. Refs github/cli#220 Co-Authored-By: Copilot <223556219+Copilot@users.noreply.github.com> --- internal/ghcmd/cmd.go | 53 +++++++++++++++++++++++++++++++++ pkg/extensions/official.go | 40 +++++++++++++++++++++++++ pkg/extensions/official_test.go | 41 +++++++++++++++++++++++++ 3 files changed, 134 insertions(+) create mode 100644 pkg/extensions/official.go create mode 100644 pkg/extensions/official_test.go diff --git a/internal/ghcmd/cmd.go b/internal/ghcmd/cmd.go index 8690078c66e..3bf0f5f4a9b 100644 --- a/internal/ghcmd/cmd.go +++ b/internal/ghcmd/cmd.go @@ -14,15 +14,18 @@ import ( surveyCore "github.com/AlecAivazis/survey/v2/core" "github.com/AlecAivazis/survey/v2/terminal" + "github.com/MakeNowJust/heredoc" "github.com/cli/cli/v2/api" "github.com/cli/cli/v2/internal/agents" "github.com/cli/cli/v2/internal/build" "github.com/cli/cli/v2/internal/config" "github.com/cli/cli/v2/internal/config/migration" + "github.com/cli/cli/v2/internal/prompter" "github.com/cli/cli/v2/internal/update" "github.com/cli/cli/v2/pkg/cmd/factory" "github.com/cli/cli/v2/pkg/cmd/root" "github.com/cli/cli/v2/pkg/cmdutil" + "github.com/cli/cli/v2/pkg/extensions" "github.com/cli/cli/v2/pkg/iostreams" "github.com/cli/cli/v2/utils" "github.com/cli/safeexec" @@ -140,6 +143,18 @@ func Main() exitCode { return exitCode(extError.ExitCode()) } + // Check if any of the provided args match a known official extension. + // We scan all args rather than just the first because global flags + // (e.g. --repo) may precede the unknown command name. + if strings.HasPrefix(err.Error(), "unknown command ") { + for _, arg := range expandedArgs { + if ext := extensions.FindOfficialExtension(arg); ext != nil { + handleOfficialExtension(cmdFactory.IOStreams, cmdFactory.Prompter, cmdFactory.ExtensionManager, ext, err) + return exitError + } + } + } + printError(stderr, err, cmd, hasDebug) if strings.Contains(err.Error(), "Incorrect function") { @@ -245,3 +260,41 @@ func isUnderHomebrew(ghBinary string) bool { brewBinPrefix := filepath.Join(strings.TrimSpace(string(brewPrefixBytes)), "bin") + string(filepath.Separator) return strings.HasPrefix(ghBinary, brewBinPrefix) } + +// handleOfficialExtension prints a suggestion for the matched official extension +// and, in interactive TTY sessions, prompts the user to install it. +func handleOfficialExtension(io *iostreams.IOStreams, p prompter.Prompter, em extensions.ExtensionManager, ext *extensions.OfficialExtension, err error) { + stderr := io.ErrOut + + fmt.Fprintln(stderr, err) + + if !io.CanPrompt() { + fmt.Fprint(stderr, heredoc.Docf(` + %q is also available as an official extension. + To install it, run: + gh extension install github.com/%s/%s + `, fmt.Sprintf("gh %s", ext.Name), ext.Owner, ext.Repo)) + return + } + + prompt := heredoc.Docf(` + %q is also available as an official extension. + Would you like to install it now? + `, fmt.Sprintf("gh %s", ext.Name)) + confirmed, promptErr := p.Confirm(prompt, true) + if promptErr != nil || !confirmed { + return + } + + repo := ext.Repository() + io.StartProgressIndicatorWithLabel(fmt.Sprintf("Installing %s/%s...", ext.Owner, ext.Repo)) + defer io.StopProgressIndicator() + installErr := em.Install(repo, "") + io.StopProgressIndicator() + if installErr != nil { + fmt.Fprintf(stderr, "Failed to install extension: %s\n", installErr) + return + } + + fmt.Fprintf(stderr, "Successfully installed %s/%s\n", ext.Owner, ext.Repo) +} diff --git a/pkg/extensions/official.go b/pkg/extensions/official.go new file mode 100644 index 00000000000..a1e6996db13 --- /dev/null +++ b/pkg/extensions/official.go @@ -0,0 +1,40 @@ +package extensions + +import ( + "github.com/cli/cli/v2/internal/ghrepo" +) + +// OfficialExtension describes a GitHub-owned CLI extension that can be +// suggested to users when they invoke an unknown command. +type OfficialExtension struct { + Name string + Owner string + Repo string +} + +// Repository returns a ghrepo.Interface pinned to github.com for use with +// ExtensionManager.Install. +func (e *OfficialExtension) Repository() ghrepo.Interface { + return ghrepo.NewWithHost(e.Owner, e.Repo, "github.com") +} + +// officialExtensions is the hard-coded registry of GitHub-owned extensions +// that gh will suggest installing when the user invokes an unknown command +// matching one of their names. +// Install suggestions include the "github.com/" host prefix so that GHES users +// install from github.com rather than their enterprise host. +var officialExtensions = []OfficialExtension{ + {Name: "aw", Owner: "github", Repo: "gh-aw"}, + {Name: "stack", Owner: "github", Repo: "gh-stack"}, +} + +// FindOfficialExtension returns the matching official extension for +// commandName, or nil if none matches. +func FindOfficialExtension(commandName string) *OfficialExtension { + for _, ext := range officialExtensions { + if ext.Name == commandName { + return &ext + } + } + return nil +} diff --git a/pkg/extensions/official_test.go b/pkg/extensions/official_test.go new file mode 100644 index 00000000000..0a0b5ec52ea --- /dev/null +++ b/pkg/extensions/official_test.go @@ -0,0 +1,41 @@ +package extensions + +import ( + "testing" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +func TestFindOfficialExtension(t *testing.T) { + tests := []struct { + name string + commandName string + wantNil bool + wantRepo string + }{ + {name: "found", commandName: "stack", wantNil: false, wantRepo: "gh-stack"}, + {name: "not found", commandName: "xyzzy", wantNil: true}, + {name: "empty", commandName: "", wantNil: true}, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + ext := FindOfficialExtension(tt.commandName) + if tt.wantNil { + assert.Nil(t, ext) + } else { + require.NotNil(t, ext) + assert.Equal(t, tt.wantRepo, ext.Repo) + } + }) + } +} + +func TestOfficialExtension_Repository(t *testing.T) { + ext := &OfficialExtension{Name: "stack", Owner: "github", Repo: "gh-stack"} + repo := ext.Repository() + assert.Equal(t, "github", repo.RepoOwner()) + assert.Equal(t, "gh-stack", repo.RepoName()) + assert.Equal(t, "github.com", repo.RepoHost()) +} From 8b115d2c2355159b9fe0ba123b81f2e5aec0fbd1 Mon Sep 17 00:00:00 2001 From: Sam Morrow Date: Thu, 16 Apr 2026 16:19:59 +0200 Subject: [PATCH 02/12] fix: address post-merge review feedback for skills commands - Remove direct opts.client injection in publish; use HttpClient factory pattern (PR #13168 feedback) - Rename testName to name in discovery test struct (PR #13170 feedback) - Use typed struct keys for dedup map with case-insensitive comparison in deduplicateResults (PR #13170 feedback) - Simplify remote selection to use Remotes() ordering instead of manual origin-first logic (PR #13171 feedback) - Fix push icon timing: show no icon before push, SuccessIcon after success (PR #13171 feedback) Closes #13184 Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- internal/skills/discovery/discovery_test.go | 18 ++++----- pkg/cmd/skills/publish/publish.go | 41 ++++++--------------- pkg/cmd/skills/publish/publish_test.go | 39 ++++++++++---------- pkg/cmd/skills/search/search.go | 17 ++++++++- 4 files changed, 55 insertions(+), 60 deletions(-) diff --git a/internal/skills/discovery/discovery_test.go b/internal/skills/discovery/discovery_test.go index 7f74c299876..f9abc593cf8 100644 --- a/internal/skills/discovery/discovery_test.go +++ b/internal/skills/discovery/discovery_test.go @@ -925,21 +925,21 @@ func TestMatchesSkillPath(t *testing.T) { func TestMatchSkillPath(t *testing.T) { tests := []struct { - testName string + name string path string wantName string wantNamespace string }{ - {testName: "skills convention", path: "skills/code-review/SKILL.md", wantName: "code-review", wantNamespace: ""}, - {testName: "namespaced convention", path: "skills/monalisa/issue-triage/SKILL.md", wantName: "issue-triage", wantNamespace: "monalisa"}, - {testName: "plugins convention", path: "plugins/hubot/skills/pr-summary/SKILL.md", wantName: "pr-summary", wantNamespace: "hubot"}, - {testName: "non-skill file", path: "README.md", wantName: "", wantNamespace: ""}, - {testName: "same name different namespace 1", path: "skills/kynan/commit/SKILL.md", wantName: "commit", wantNamespace: "kynan"}, - {testName: "same name different namespace 2", path: "skills/will/commit/SKILL.md", wantName: "commit", wantNamespace: "will"}, - {testName: "root convention", path: "my-skill/SKILL.md", wantName: "my-skill", wantNamespace: ""}, + {name: "skills convention", path: "skills/code-review/SKILL.md", wantName: "code-review", wantNamespace: ""}, + {name: "namespaced convention", path: "skills/monalisa/issue-triage/SKILL.md", wantName: "issue-triage", wantNamespace: "monalisa"}, + {name: "plugins convention", path: "plugins/hubot/skills/pr-summary/SKILL.md", wantName: "pr-summary", wantNamespace: "hubot"}, + {name: "non-skill file", path: "README.md", wantName: "", wantNamespace: ""}, + {name: "same name different namespace 1", path: "skills/kynan/commit/SKILL.md", wantName: "commit", wantNamespace: "kynan"}, + {name: "same name different namespace 2", path: "skills/will/commit/SKILL.md", wantName: "commit", wantNamespace: "will"}, + {name: "root convention", path: "my-skill/SKILL.md", wantName: "my-skill", wantNamespace: ""}, } for _, tt := range tests { - t.Run(tt.testName, func(t *testing.T) { + t.Run(tt.name, func(t *testing.T) { name, namespace := MatchSkillPath(tt.path) assert.Equal(t, tt.wantName, name) assert.Equal(t, tt.wantNamespace, namespace) diff --git a/pkg/cmd/skills/publish/publish.go b/pkg/cmd/skills/publish/publish.go index 213afeba53d..d8278187694 100644 --- a/pkg/cmd/skills/publish/publish.go +++ b/pkg/cmd/skills/publish/publish.go @@ -42,8 +42,7 @@ type PublishOptions struct { DryRun bool Tag string - client *api.Client // injectable for tests; nil means use factory - host string // resolved from config in production + host string // resolved from config in production } // publishDiagnostic is a single validation finding. @@ -178,11 +177,10 @@ func publishRun(opts *PublishOptions) error { canPrompt := opts.IO.CanPrompt() - // Use injected client or create one from the factory HttpClient. - // Initialization is deferred until after local validation so that + // Client initialization is deferred until after local validation so that // simple errors (missing skills/, bad SKILL.md, etc.) are reported // without requiring an HTTP client. - client := opts.client + var client *api.Client host := opts.host var diagnostics []publishDiagnostic @@ -343,14 +341,11 @@ func publishRun(opts *PublishOptions) error { hasTopic := false var existingTags []tagEntry if owner != "" && repo != "" { - // Create API client from factory if not already injected (tests inject directly). - if client == nil { - httpClient, err := opts.HttpClient() - if err != nil { - return err - } - client = api.NewClientFromHTTP(httpClient) + httpClient, err := opts.HttpClient() + if err != nil { + return err } + client = api.NewClientFromHTTP(httpClient) if host == "" && repoInfo != nil { host = repoInfo.Repo.RepoHost() @@ -658,10 +653,11 @@ func ensurePushed(opts *PublishOptions, dir, remoteName string) error { } ref := fmt.Sprintf("HEAD:refs/heads/%s", currentBranch) - fmt.Fprintf(opts.IO.ErrOut, "%s Pushing %s to %s\n", cs.SuccessIcon(), currentBranch, remoteName) + fmt.Fprintf(opts.IO.ErrOut, "Pushing %s to %s...\n", currentBranch, remoteName) if err := gitClient.Push(ctx, remoteName, ref); err != nil { return fmt.Errorf("failed to push branch %s: %w", currentBranch, err) } + fmt.Fprintf(opts.IO.ErrOut, "%s Pushed %s to %s\n", cs.SuccessIcon(), currentBranch, remoteName) return nil } @@ -924,7 +920,9 @@ type gitHubRemote struct { } // detectGitHubRemote attempts to detect the GitHub owner/repo from git remotes -// in the given directory. +// in the given directory. Remotes are tried in the order returned by +// gitClient.Remotes (upstream > github > origin > rest), so the first +// GitHub-pointing remote wins. func detectGitHubRemote(gitClient *git.Client, dir string) (*gitHubRemote, error) { if gitClient == nil { return nil, nil @@ -933,26 +931,11 @@ func detectGitHubRemote(gitClient *git.Client, dir string) (*gitHubRemote, error dirClient := gitClient.Copy() dirClient.RepoDir = dir - // Try origin first - if url, err := dirClient.RemoteURL(context.Background(), "origin"); err == nil { - repo, parseErr := parseGitHubURL(url) - if parseErr != nil { - return nil, parseErr - } - if repo != nil { - return &gitHubRemote{Repo: repo, RemoteName: "origin"}, nil - } - } - - // Fall back to any remote that points to GitHub remotes, err := dirClient.Remotes(context.Background()) if err != nil { return nil, nil //nolint:nilerr // failing to list remotes is not an error; it just means no repo detected } for _, r := range remotes { - if r.Name == "origin" { - continue - } if url, err := dirClient.RemoteURL(context.Background(), r.Name); err == nil { repo, parseErr := parseGitHubURL(url) if parseErr != nil { diff --git a/pkg/cmd/skills/publish/publish_test.go b/pkg/cmd/skills/publish/publish_test.go index 8c7205a3deb..27bdfb7684c 100644 --- a/pkg/cmd/skills/publish/publish_test.go +++ b/pkg/cmd/skills/publish/publish_test.go @@ -10,7 +10,6 @@ import ( "testing" "github.com/MakeNowJust/heredoc" - "github.com/cli/cli/v2/api" "github.com/cli/cli/v2/git" "github.com/cli/cli/v2/internal/prompter" "github.com/cli/cli/v2/pkg/cmdutil" @@ -165,7 +164,7 @@ func TestPublishRun_UnsupportedHost(t *testing.T) { IO: ios, Dir: dir, GitClient: &git.Client{}, - client: api.NewClientFromHTTP(&http.Client{}), + HttpClient: func() (*http.Client, error) { return &http.Client{}, nil }, host: "acme.ghes.com", }) require.ErrorContains(t, err, "supports only github.com") @@ -290,7 +289,7 @@ func TestPublishRun(t *testing.T) { Dir: dir, DryRun: true, GitClient: &git.Client{}, - client: api.NewClientFromHTTP(&http.Client{Transport: reg}), + HttpClient: func() (*http.Client, error) { return &http.Client{Transport: reg}, nil }, host: "github.com", } }, @@ -327,7 +326,7 @@ func TestPublishRun(t *testing.T) { Dir: dir, DryRun: true, GitClient: &git.Client{}, - client: api.NewClientFromHTTP(&http.Client{Transport: reg}), + HttpClient: func() (*http.Client, error) { return &http.Client{Transport: reg}, nil }, host: "github.com", } }, @@ -361,7 +360,7 @@ func TestPublishRun(t *testing.T) { Dir: dir, DryRun: true, GitClient: &git.Client{}, - client: api.NewClientFromHTTP(&http.Client{Transport: reg}), + HttpClient: func() (*http.Client, error) { return &http.Client{Transport: reg}, nil }, host: "github.com", } }, @@ -417,7 +416,7 @@ func TestPublishRun(t *testing.T) { ConfirmFunc: func(msg string, def bool) (bool, error) { return true, nil }, }, GitClient: &git.Client{}, - client: api.NewClientFromHTTP(&http.Client{Transport: reg}), + HttpClient: func() (*http.Client, error) { return &http.Client{Transport: reg}, nil }, host: "github.com", } }, @@ -567,7 +566,7 @@ func TestPublishRun(t *testing.T) { IO: ios, Dir: dir, GitClient: &git.Client{}, - client: api.NewClientFromHTTP(&http.Client{Transport: reg}), + HttpClient: func() (*http.Client, error) { return &http.Client{Transport: reg}, nil }, host: "github.com", } }, @@ -620,7 +619,7 @@ func TestPublishRun(t *testing.T) { IO: ios, Dir: dir, GitClient: &git.Client{}, - client: api.NewClientFromHTTP(&http.Client{Transport: reg}), + HttpClient: func() (*http.Client, error) { return &http.Client{Transport: reg}, nil }, host: "github.com", } }, @@ -684,7 +683,7 @@ func TestPublishRun(t *testing.T) { Dir: dir, DryRun: true, GitClient: &git.Client{}, - client: api.NewClientFromHTTP(&http.Client{Transport: reg}), + HttpClient: func() (*http.Client, error) { return &http.Client{Transport: reg}, nil }, host: "github.com", } }, @@ -749,7 +748,7 @@ func TestPublishRun(t *testing.T) { Dir: dir, DryRun: true, GitClient: &git.Client{}, - client: api.NewClientFromHTTP(&http.Client{Transport: reg}), + HttpClient: func() (*http.Client, error) { return &http.Client{Transport: reg}, nil }, host: "github.com", } }, @@ -899,7 +898,7 @@ func TestPublishRun(t *testing.T) { Dir: dir, DryRun: true, GitClient: &git.Client{}, - client: api.NewClientFromHTTP(&http.Client{Transport: reg}), + HttpClient: func() (*http.Client, error) { return &http.Client{Transport: reg}, nil }, host: "github.com", } }, @@ -988,7 +987,7 @@ func TestPublishRun(t *testing.T) { ConfirmFunc: func(msg string, def bool) (bool, error) { return true, nil }, }, GitClient: &git.Client{}, - client: api.NewClientFromHTTP(&http.Client{Transport: reg}), + HttpClient: func() (*http.Client, error) { return &http.Client{Transport: reg}, nil }, host: "github.com", } }, @@ -1063,7 +1062,7 @@ func TestPublishRun(t *testing.T) { Dir: dir, Tag: "v2.3.5", GitClient: &git.Client{}, - client: api.NewClientFromHTTP(&http.Client{Transport: reg}), + HttpClient: func() (*http.Client, error) { return &http.Client{Transport: reg}, nil }, host: "github.com", } }, @@ -1095,7 +1094,7 @@ func TestPublishRun(t *testing.T) { Dir: dir, Tag: "v1.0.0", // same as stubAllSecureRemote's existing tag GitClient: &git.Client{}, - client: api.NewClientFromHTTP(&http.Client{Transport: reg}), + HttpClient: func() (*http.Client, error) { return &http.Client{Transport: reg}, nil }, host: "github.com", } }, @@ -1127,7 +1126,7 @@ func TestPublishRun(t *testing.T) { IO: ios, Dir: dir, GitClient: &git.Client{}, - client: api.NewClientFromHTTP(&http.Client{Transport: reg}), + HttpClient: func() (*http.Client, error) { return &http.Client{Transport: reg}, nil }, host: "github.com", } }, @@ -1238,7 +1237,7 @@ func TestPublishRun(t *testing.T) { }, }, GitClient: &git.Client{}, - client: api.NewClientFromHTTP(&http.Client{Transport: reg}), + HttpClient: func() (*http.Client, error) { return &http.Client{Transport: reg}, nil }, host: "github.com", } }, @@ -1295,7 +1294,7 @@ func TestPublishRun(t *testing.T) { }, }, GitClient: &git.Client{}, - client: api.NewClientFromHTTP(&http.Client{Transport: reg}), + HttpClient: func() (*http.Client, error) { return &http.Client{Transport: reg}, nil }, host: "github.com", } }, @@ -1351,7 +1350,7 @@ func TestPublishRun(t *testing.T) { }, }, GitClient: &git.Client{}, - client: api.NewClientFromHTTP(&http.Client{Transport: reg}), + HttpClient: func() (*http.Client, error) { return &http.Client{Transport: reg}, nil }, host: "github.com", } }, @@ -1415,7 +1414,7 @@ func TestPublishRun(t *testing.T) { }, }, GitClient: &git.Client{}, - client: api.NewClientFromHTTP(&http.Client{Transport: reg}), + HttpClient: func() (*http.Client, error) { return &http.Client{Transport: reg}, nil }, host: "github.com", } }, @@ -1532,7 +1531,7 @@ func TestPublishRun_DirArgUsesTargetRemote(t *testing.T) { Dir: targetRepo, DryRun: true, GitClient: &git.Client{RepoDir: cwdRepo}, - client: api.NewClientFromHTTP(&http.Client{Transport: reg}), + HttpClient: func() (*http.Client, error) { return &http.Client{Transport: reg}, nil }, host: "github.com", }) diff --git a/pkg/cmd/skills/search/search.go b/pkg/cmd/skills/search/search.go index f7d4975a7ee..18471cc954f 100644 --- a/pkg/cmd/skills/search/search.go +++ b/pkg/cmd/skills/search/search.go @@ -770,9 +770,18 @@ func fetchPrimaryPages(client *api.Client, host, query string, displayPage, disp return allItems, totalCount, nil } +// skillResultKey is a typed map key for deduplicating code search results +// by (repo, namespace, skill name). All fields are lowercased for +// case-insensitive comparison. +type skillResultKey struct { + repo string + namespace string + skillName string +} + // deduplicateResults extracts unique (repo, namespace, skill name) triples from code search hits. func deduplicateResults(items []codeSearchItem) []skillResult { - seen := make(map[string]struct{}) + seen := make(map[skillResultKey]struct{}) var results []skillResult for _, item := range items { @@ -780,7 +789,11 @@ func deduplicateResults(items []codeSearchItem) []skillResult { if skillName == "" { continue } - key := item.Repository.FullName + "/" + namespace + "/" + skillName + key := skillResultKey{ + repo: strings.ToLower(item.Repository.FullName), + namespace: strings.ToLower(namespace), + skillName: strings.ToLower(skillName), + } if _, ok := seen[key]; ok { continue } From 9d00ecc24831eff164cae77fd146a0a0e9b7356e Mon Sep 17 00:00:00 2001 From: Sam Morrow Date: Thu, 16 Apr 2026 17:04:12 +0200 Subject: [PATCH 03/12] style: fix gofmt alignment in publish tests Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- pkg/cmd/skills/publish/publish_test.go | 146 ++++++++++++------------- 1 file changed, 73 insertions(+), 73 deletions(-) diff --git a/pkg/cmd/skills/publish/publish_test.go b/pkg/cmd/skills/publish/publish_test.go index 27bdfb7684c..04f400733ef 100644 --- a/pkg/cmd/skills/publish/publish_test.go +++ b/pkg/cmd/skills/publish/publish_test.go @@ -161,11 +161,11 @@ func TestPublishRun_UnsupportedHost(t *testing.T) { ios, _, _, _ := iostreams.Test() initGitRepo(t, dir, map[string]string{"origin": "https://github.com/monalisa/skills-repo.git"}) err := publishRun(&PublishOptions{ - IO: ios, - Dir: dir, - GitClient: &git.Client{}, + IO: ios, + Dir: dir, + GitClient: &git.Client{}, HttpClient: func() (*http.Client, error) { return &http.Client{}, nil }, - host: "acme.ghes.com", + host: "acme.ghes.com", }) require.ErrorContains(t, err, "supports only github.com") } @@ -285,12 +285,12 @@ func TestPublishRun(t *testing.T) { opts: func(ios *iostreams.IOStreams, dir string, reg *httpmock.Registry) *PublishOptions { t.Helper() return &PublishOptions{ - IO: ios, - Dir: dir, - DryRun: true, - GitClient: &git.Client{}, + IO: ios, + Dir: dir, + DryRun: true, + GitClient: &git.Client{}, HttpClient: func() (*http.Client, error) { return &http.Client{Transport: reg}, nil }, - host: "github.com", + host: "github.com", } }, wantStdout: "1 skill(s) validated successfully", @@ -322,12 +322,12 @@ func TestPublishRun(t *testing.T) { opts: func(ios *iostreams.IOStreams, dir string, reg *httpmock.Registry) *PublishOptions { t.Helper() return &PublishOptions{ - IO: ios, - Dir: dir, - DryRun: true, - GitClient: &git.Client{}, + IO: ios, + Dir: dir, + DryRun: true, + GitClient: &git.Client{}, HttpClient: func() (*http.Client, error) { return &http.Client{Transport: reg}, nil }, - host: "github.com", + host: "github.com", } }, wantStdout: "1 skill(s) validated successfully", @@ -356,12 +356,12 @@ func TestPublishRun(t *testing.T) { "origin": "https://github.com/monalisa/skills-repo.git", }) return &PublishOptions{ - IO: ios, - Dir: dir, - DryRun: true, - GitClient: &git.Client{}, + IO: ios, + Dir: dir, + DryRun: true, + GitClient: &git.Client{}, HttpClient: func() (*http.Client, error) { return &http.Client{Transport: reg}, nil }, - host: "github.com", + host: "github.com", } }, wantStdout: "1 skill(s) validated successfully", @@ -415,9 +415,9 @@ func TestPublishRun(t *testing.T) { Prompter: &prompter.PrompterMock{ ConfirmFunc: func(msg string, def bool) (bool, error) { return true, nil }, }, - GitClient: &git.Client{}, + GitClient: &git.Client{}, HttpClient: func() (*http.Client, error) { return &http.Client{Transport: reg}, nil }, - host: "github.com", + host: "github.com", } }, wantStdout: "Published v1.0.1", @@ -563,11 +563,11 @@ func TestPublishRun(t *testing.T) { "origin": "https://github.com/octocat/secure-repo.git", }) return &PublishOptions{ - IO: ios, - Dir: dir, - GitClient: &git.Client{}, + IO: ios, + Dir: dir, + GitClient: &git.Client{}, HttpClient: func() (*http.Client, error) { return &http.Client{Transport: reg}, nil }, - host: "github.com", + host: "github.com", } }, wantStdout: "secret scanning is not enabled", @@ -616,11 +616,11 @@ func TestPublishRun(t *testing.T) { "origin": "https://github.com/octocat/tag-repo.git", }) return &PublishOptions{ - IO: ios, - Dir: dir, - GitClient: &git.Client{}, + IO: ios, + Dir: dir, + GitClient: &git.Client{}, HttpClient: func() (*http.Client, error) { return &http.Client{Transport: reg}, nil }, - host: "github.com", + host: "github.com", } }, wantStdout: "tag protection", @@ -679,12 +679,12 @@ func TestPublishRun(t *testing.T) { "origin": "https://github.com/octocat/code-repo.git", }) return &PublishOptions{ - IO: ios, - Dir: dir, - DryRun: true, - GitClient: &git.Client{}, + IO: ios, + Dir: dir, + DryRun: true, + GitClient: &git.Client{}, HttpClient: func() (*http.Client, error) { return &http.Client{Transport: reg}, nil }, - host: "github.com", + host: "github.com", } }, wantStderr: "code scanning", @@ -744,12 +744,12 @@ func TestPublishRun(t *testing.T) { "origin": "https://github.com/octocat/dep-repo.git", }) return &PublishOptions{ - IO: ios, - Dir: dir, - DryRun: true, - GitClient: &git.Client{}, + IO: ios, + Dir: dir, + DryRun: true, + GitClient: &git.Client{}, HttpClient: func() (*http.Client, error) { return &http.Client{Transport: reg}, nil }, - host: "github.com", + host: "github.com", } }, wantStderr: "Dependabot", @@ -894,12 +894,12 @@ func TestPublishRun(t *testing.T) { "upstream": "git@github.com:octocat/repo.git", }) return &PublishOptions{ - IO: ios, - Dir: dir, - DryRun: true, - GitClient: &git.Client{}, + IO: ios, + Dir: dir, + DryRun: true, + GitClient: &git.Client{}, HttpClient: func() (*http.Client, error) { return &http.Client{Transport: reg}, nil }, - host: "github.com", + host: "github.com", } }, wantStderr: "octocat/repo", @@ -986,9 +986,9 @@ func TestPublishRun(t *testing.T) { Prompter: &prompter.PrompterMock{ ConfirmFunc: func(msg string, def bool) (bool, error) { return true, nil }, }, - GitClient: &git.Client{}, + GitClient: &git.Client{}, HttpClient: func() (*http.Client, error) { return &http.Client{Transport: reg}, nil }, - host: "github.com", + host: "github.com", } }, wantStdout: "Added \"agent-skills\" topic", @@ -1058,12 +1058,12 @@ func TestPublishRun(t *testing.T) { "origin": "https://github.com/monalisa/skills-repo.git", }) return &PublishOptions{ - IO: ios, - Dir: dir, - Tag: "v2.3.5", - GitClient: &git.Client{}, + IO: ios, + Dir: dir, + Tag: "v2.3.5", + GitClient: &git.Client{}, HttpClient: func() (*http.Client, error) { return &http.Client{Transport: reg}, nil }, - host: "github.com", + host: "github.com", } }, wantStdout: "Published v2.3.5", @@ -1090,12 +1090,12 @@ func TestPublishRun(t *testing.T) { "origin": "https://github.com/monalisa/skills-repo.git", }) return &PublishOptions{ - IO: ios, - Dir: dir, - Tag: "v1.0.0", // same as stubAllSecureRemote's existing tag - GitClient: &git.Client{}, + IO: ios, + Dir: dir, + Tag: "v1.0.0", // same as stubAllSecureRemote's existing tag + GitClient: &git.Client{}, HttpClient: func() (*http.Client, error) { return &http.Client{Transport: reg}, nil }, - host: "github.com", + host: "github.com", } }, wantErr: "tag v1.0.0 already exists", @@ -1123,11 +1123,11 @@ func TestPublishRun(t *testing.T) { "origin": "https://github.com/monalisa/skills-repo.git", }) return &PublishOptions{ - IO: ios, - Dir: dir, - GitClient: &git.Client{}, + IO: ios, + Dir: dir, + GitClient: &git.Client{}, HttpClient: func() (*http.Client, error) { return &http.Client{Transport: reg}, nil }, - host: "github.com", + host: "github.com", } }, wantStdout: "ok", @@ -1236,9 +1236,9 @@ func TestPublishRun(t *testing.T) { return "v1.0.0", nil // accept suggested tag }, }, - GitClient: &git.Client{}, + GitClient: &git.Client{}, HttpClient: func() (*http.Client, error) { return &http.Client{Transport: reg}, nil }, - host: "github.com", + host: "github.com", } }, wantStdout: "Published v1.0.0", @@ -1293,9 +1293,9 @@ func TestPublishRun(t *testing.T) { return "beta-1", nil }, }, - GitClient: &git.Client{}, + GitClient: &git.Client{}, HttpClient: func() (*http.Client, error) { return &http.Client{Transport: reg}, nil }, - host: "github.com", + host: "github.com", } }, wantStdout: "Published beta-1", @@ -1349,9 +1349,9 @@ func TestPublishRun(t *testing.T) { return "v1.0.1", nil }, }, - GitClient: &git.Client{}, + GitClient: &git.Client{}, HttpClient: func() (*http.Client, error) { return &http.Client{Transport: reg}, nil }, - host: "github.com", + host: "github.com", } }, wantErr: "CancelError", @@ -1413,9 +1413,9 @@ func TestPublishRun(t *testing.T) { return "v1.0.1", nil }, }, - GitClient: &git.Client{}, + GitClient: &git.Client{}, HttpClient: func() (*http.Client, error) { return &http.Client{Transport: reg}, nil }, - host: "github.com", + host: "github.com", } }, wantStdout: "Enabled immutable releases", @@ -1527,12 +1527,12 @@ func TestPublishRun_DirArgUsesTargetRemote(t *testing.T) { stubAllSecureRemote(reg, "monalisa", "target-repo") err := publishRun(&PublishOptions{ - IO: ios, - Dir: targetRepo, - DryRun: true, - GitClient: &git.Client{RepoDir: cwdRepo}, + IO: ios, + Dir: targetRepo, + DryRun: true, + GitClient: &git.Client{RepoDir: cwdRepo}, HttpClient: func() (*http.Client, error) { return &http.Client{Transport: reg}, nil }, - host: "github.com", + host: "github.com", }) require.NoError(t, err) From 7ad1d7c0a16be0585f018e4bfa170336d61b8c8e Mon Sep 17 00:00:00 2001 From: William Martin Date: Thu, 16 Apr 2026 13:14:10 +0200 Subject: [PATCH 04/12] Suggest and install official extensions via stub commands Register hidden stub commands for official GitHub extensions (gh-aw, gh-stack) that offer to install the extension when invoked. This replaces the error-string-matching approach from the original PR with proper cobra commands that: - Avoid false-positive matches on flag values or post-'--' args - Eliminate conflicting cobra 'Did you mean?' suggestions - Properly propagate prompt/install errors for correct exit codes - Are hidden from help output and shell completions - Use GroupID "extension" so checkValidExtension allows installing over them - Are registered after extensions and aliases so both take priority Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- internal/ghcmd/cmd.go | 53 -------- pkg/cmd/root/official_extension.go | 76 +++++++++++ pkg/cmd/root/official_extension_test.go | 168 ++++++++++++++++++++++++ pkg/cmd/root/root.go | 12 ++ pkg/extensions/official.go | 24 +--- pkg/extensions/official_test.go | 26 ---- 6 files changed, 261 insertions(+), 98 deletions(-) create mode 100644 pkg/cmd/root/official_extension.go create mode 100644 pkg/cmd/root/official_extension_test.go diff --git a/internal/ghcmd/cmd.go b/internal/ghcmd/cmd.go index 3bf0f5f4a9b..8690078c66e 100644 --- a/internal/ghcmd/cmd.go +++ b/internal/ghcmd/cmd.go @@ -14,18 +14,15 @@ import ( surveyCore "github.com/AlecAivazis/survey/v2/core" "github.com/AlecAivazis/survey/v2/terminal" - "github.com/MakeNowJust/heredoc" "github.com/cli/cli/v2/api" "github.com/cli/cli/v2/internal/agents" "github.com/cli/cli/v2/internal/build" "github.com/cli/cli/v2/internal/config" "github.com/cli/cli/v2/internal/config/migration" - "github.com/cli/cli/v2/internal/prompter" "github.com/cli/cli/v2/internal/update" "github.com/cli/cli/v2/pkg/cmd/factory" "github.com/cli/cli/v2/pkg/cmd/root" "github.com/cli/cli/v2/pkg/cmdutil" - "github.com/cli/cli/v2/pkg/extensions" "github.com/cli/cli/v2/pkg/iostreams" "github.com/cli/cli/v2/utils" "github.com/cli/safeexec" @@ -143,18 +140,6 @@ func Main() exitCode { return exitCode(extError.ExitCode()) } - // Check if any of the provided args match a known official extension. - // We scan all args rather than just the first because global flags - // (e.g. --repo) may precede the unknown command name. - if strings.HasPrefix(err.Error(), "unknown command ") { - for _, arg := range expandedArgs { - if ext := extensions.FindOfficialExtension(arg); ext != nil { - handleOfficialExtension(cmdFactory.IOStreams, cmdFactory.Prompter, cmdFactory.ExtensionManager, ext, err) - return exitError - } - } - } - printError(stderr, err, cmd, hasDebug) if strings.Contains(err.Error(), "Incorrect function") { @@ -260,41 +245,3 @@ func isUnderHomebrew(ghBinary string) bool { brewBinPrefix := filepath.Join(strings.TrimSpace(string(brewPrefixBytes)), "bin") + string(filepath.Separator) return strings.HasPrefix(ghBinary, brewBinPrefix) } - -// handleOfficialExtension prints a suggestion for the matched official extension -// and, in interactive TTY sessions, prompts the user to install it. -func handleOfficialExtension(io *iostreams.IOStreams, p prompter.Prompter, em extensions.ExtensionManager, ext *extensions.OfficialExtension, err error) { - stderr := io.ErrOut - - fmt.Fprintln(stderr, err) - - if !io.CanPrompt() { - fmt.Fprint(stderr, heredoc.Docf(` - %q is also available as an official extension. - To install it, run: - gh extension install github.com/%s/%s - `, fmt.Sprintf("gh %s", ext.Name), ext.Owner, ext.Repo)) - return - } - - prompt := heredoc.Docf(` - %q is also available as an official extension. - Would you like to install it now? - `, fmt.Sprintf("gh %s", ext.Name)) - confirmed, promptErr := p.Confirm(prompt, true) - if promptErr != nil || !confirmed { - return - } - - repo := ext.Repository() - io.StartProgressIndicatorWithLabel(fmt.Sprintf("Installing %s/%s...", ext.Owner, ext.Repo)) - defer io.StopProgressIndicator() - installErr := em.Install(repo, "") - io.StopProgressIndicator() - if installErr != nil { - fmt.Fprintf(stderr, "Failed to install extension: %s\n", installErr) - return - } - - fmt.Fprintf(stderr, "Successfully installed %s/%s\n", ext.Owner, ext.Repo) -} diff --git a/pkg/cmd/root/official_extension.go b/pkg/cmd/root/official_extension.go new file mode 100644 index 00000000000..cde6bb6da40 --- /dev/null +++ b/pkg/cmd/root/official_extension.go @@ -0,0 +1,76 @@ +package root + +import ( + "fmt" + + "github.com/MakeNowJust/heredoc" + "github.com/cli/cli/v2/internal/prompter" + "github.com/cli/cli/v2/pkg/extensions" + "github.com/cli/cli/v2/pkg/iostreams" + "github.com/spf13/cobra" +) + +// NewCmdOfficialExtension creates a hidden stub command for an official +// extension that has not yet been installed. When invoked, it suggests +// installing the extension and, in interactive sessions, offers to do so +// immediately. After a successful install, the extension is dispatched with +// the original arguments. +func NewCmdOfficialExtension(io *iostreams.IOStreams, p prompter.Prompter, em extensions.ExtensionManager, ext *extensions.OfficialExtension) *cobra.Command { + return &cobra.Command{ + Use: ext.Name, + Short: fmt.Sprintf("Install the official %s extension", ext.Name), + Hidden: true, + GroupID: "extension", + Annotations: map[string]string{ + "skipAuthCheck": "true", + }, + // Accept any args/flags the user may have passed so we don't get + // cobra validation errors before reaching RunE. + DisableFlagParsing: true, + RunE: func(cmd *cobra.Command, args []string) error { + return officialExtensionRun(io, p, em, ext, args) + }, + } +} + +func officialExtensionRun(io *iostreams.IOStreams, p prompter.Prompter, em extensions.ExtensionManager, ext *extensions.OfficialExtension, args []string) error { + stderr := io.ErrOut + + if !io.CanPrompt() { + fmt.Fprint(stderr, heredoc.Docf(` + %[1]s is available as an official extension. + To install it, run: + gh extension install %[2]s/%[3]s + `, fmt.Sprintf("gh %s", ext.Name), ext.Owner, ext.Repo)) + return nil + } + + prompt := heredoc.Docf(` + %[1]s is available as an official extension. + Would you like to install it now? + `, fmt.Sprintf("gh %s", ext.Name)) + confirmed, err := p.Confirm(prompt, true) + if err != nil { + return err + } + if !confirmed { + return nil + } + + repo := ext.Repository() + io.StartProgressIndicatorWithLabel(fmt.Sprintf("Installing %s/%s...", ext.Owner, ext.Repo)) + installErr := em.Install(repo, "") + io.StopProgressIndicator() + if installErr != nil { + return fmt.Errorf("failed to install extension: %w", installErr) + } + + fmt.Fprintf(stderr, "Successfully installed %s/%s\n", ext.Owner, ext.Repo) + + // Dispatch the newly installed extension with the original arguments. + dispatchArgs := append([]string{ext.Name}, args...) + if _, dispatchErr := em.Dispatch(dispatchArgs, io.In, io.Out, stderr); dispatchErr != nil { + return dispatchErr + } + return nil +} diff --git a/pkg/cmd/root/official_extension_test.go b/pkg/cmd/root/official_extension_test.go new file mode 100644 index 00000000000..8b6aaa3ae14 --- /dev/null +++ b/pkg/cmd/root/official_extension_test.go @@ -0,0 +1,168 @@ +package root + +import ( + "fmt" + "io" + "testing" + + "github.com/cli/cli/v2/internal/ghrepo" + "github.com/cli/cli/v2/internal/prompter" + "github.com/cli/cli/v2/pkg/extensions" + "github.com/cli/cli/v2/pkg/iostreams" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +func TestOfficialExtensionRun_NonTTY(t *testing.T) { + ios, _, _, stderr := iostreams.Test() + // non-TTY by default + + ext := &extensions.OfficialExtension{Name: "stack", Owner: "github", Repo: "gh-stack"} + em := &extensions.ExtensionManagerMock{} + p := &prompter.PrompterMock{} + + err := officialExtensionRun(ios, p, em, ext, nil) + require.NoError(t, err) + + assert.Contains(t, stderr.String(), "gh stack") + assert.Contains(t, stderr.String(), "gh extension install github/gh-stack") +} + +func TestOfficialExtensionRun_TTY_Confirmed(t *testing.T) { + ios, _, _, stderr := iostreams.Test() + ios.SetStdinTTY(true) + ios.SetStdoutTTY(true) + ios.SetStderrTTY(true) + + ext := &extensions.OfficialExtension{Name: "stack", Owner: "github", Repo: "gh-stack"} + var installedRepo ghrepo.Interface + var dispatchedArgs []string + em := &extensions.ExtensionManagerMock{ + InstallFunc: func(repo ghrepo.Interface, pin string) error { + installedRepo = repo + return nil + }, + DispatchFunc: func(args []string, stdin io.Reader, stdout, stderr io.Writer) (bool, error) { + dispatchedArgs = args + return true, nil + }, + } + p := &prompter.PrompterMock{ + ConfirmFunc: func(_ string, _ bool) (bool, error) { + return true, nil + }, + } + + err := officialExtensionRun(ios, p, em, ext, []string{"--help"}) + require.NoError(t, err) + + require.NotNil(t, installedRepo) + assert.Equal(t, "github", installedRepo.RepoOwner()) + assert.Equal(t, "gh-stack", installedRepo.RepoName()) + assert.Equal(t, "github.com", installedRepo.RepoHost()) + assert.Contains(t, stderr.String(), "Successfully installed github/gh-stack") + assert.Equal(t, []string{"stack", "--help"}, dispatchedArgs) +} + +func TestOfficialExtensionRun_TTY_Declined(t *testing.T) { + ios, _, _, _ := iostreams.Test() + ios.SetStdinTTY(true) + ios.SetStdoutTTY(true) + ios.SetStderrTTY(true) + + ext := &extensions.OfficialExtension{Name: "stack", Owner: "github", Repo: "gh-stack"} + em := &extensions.ExtensionManagerMock{} + p := &prompter.PrompterMock{ + ConfirmFunc: func(_ string, _ bool) (bool, error) { + return false, nil + }, + } + + err := officialExtensionRun(ios, p, em, ext, nil) + require.NoError(t, err) + + assert.Empty(t, em.InstallCalls()) +} + +func TestOfficialExtensionRun_TTY_PromptError(t *testing.T) { + ios, _, _, _ := iostreams.Test() + ios.SetStdinTTY(true) + ios.SetStdoutTTY(true) + ios.SetStderrTTY(true) + + ext := &extensions.OfficialExtension{Name: "stack", Owner: "github", Repo: "gh-stack"} + em := &extensions.ExtensionManagerMock{} + p := &prompter.PrompterMock{ + ConfirmFunc: func(_ string, _ bool) (bool, error) { + return false, fmt.Errorf("prompt interrupted") + }, + } + + err := officialExtensionRun(ios, p, em, ext, nil) + require.Error(t, err) + assert.Contains(t, err.Error(), "prompt interrupted") +} + +func TestOfficialExtensionRun_TTY_InstallError(t *testing.T) { + ios, _, _, _ := iostreams.Test() + ios.SetStdinTTY(true) + ios.SetStdoutTTY(true) + ios.SetStderrTTY(true) + + ext := &extensions.OfficialExtension{Name: "stack", Owner: "github", Repo: "gh-stack"} + em := &extensions.ExtensionManagerMock{ + InstallFunc: func(_ ghrepo.Interface, _ string) error { + return fmt.Errorf("network error") + }, + } + p := &prompter.PrompterMock{ + ConfirmFunc: func(_ string, _ bool) (bool, error) { + return true, nil + }, + } + + err := officialExtensionRun(ios, p, em, ext, nil) + require.Error(t, err) + assert.Contains(t, err.Error(), "network error") +} + +func TestOfficialExtensionRun_TTY_DispatchError(t *testing.T) { + ios, _, _, _ := iostreams.Test() + ios.SetStdinTTY(true) + ios.SetStdoutTTY(true) + ios.SetStderrTTY(true) + + ext := &extensions.OfficialExtension{Name: "stack", Owner: "github", Repo: "gh-stack"} + em := &extensions.ExtensionManagerMock{ + InstallFunc: func(_ ghrepo.Interface, _ string) error { + return nil + }, + DispatchFunc: func(_ []string, _ io.Reader, _, _ io.Writer) (bool, error) { + return false, fmt.Errorf("dispatch failed") + }, + } + p := &prompter.PrompterMock{ + ConfirmFunc: func(_ string, _ bool) (bool, error) { + return true, nil + }, + } + + err := officialExtensionRun(ios, p, em, ext, nil) + require.Error(t, err) + assert.Contains(t, err.Error(), "dispatch failed") +} + +func TestNewCmdOfficialExtension_Properties(t *testing.T) { + ios, _, _, _ := iostreams.Test() + ext := &extensions.OfficialExtension{Name: "stack", Owner: "github", Repo: "gh-stack"} + em := &extensions.ExtensionManagerMock{} + p := &prompter.PrompterMock{} + + cmd := NewCmdOfficialExtension(ios, p, em, ext) + + assert.Equal(t, "stack", cmd.Use) + assert.True(t, cmd.Hidden) + assert.Equal(t, "extension", cmd.GroupID) + assert.True(t, cmd.DisableFlagParsing) + assert.Equal(t, "true", cmd.Annotations["skipAuthCheck"]) +} diff --git a/pkg/cmd/root/root.go b/pkg/cmd/root/root.go index ed33f568ed3..793b3bc8320 100644 --- a/pkg/cmd/root/root.go +++ b/pkg/cmd/root/root.go @@ -44,6 +44,7 @@ import ( versionCmd "github.com/cli/cli/v2/pkg/cmd/version" workflowCmd "github.com/cli/cli/v2/pkg/cmd/workflow" "github.com/cli/cli/v2/pkg/cmdutil" + "github.com/cli/cli/v2/pkg/extensions" "github.com/google/shlex" "github.com/spf13/cobra" ) @@ -229,6 +230,17 @@ func NewCmdRoot(f *cmdutil.Factory, version, buildDate string) (*cobra.Command, } } + // Official extension stubs — hidden commands that suggest installing + // GitHub-owned extensions when invoked. Registered after real extensions + // and aliases so that both take priority over stubs. + for i := range extensions.OfficialExtensions { + ext := &extensions.OfficialExtensions[i] + if _, _, err := cmd.Find([]string{ext.Name}); err == nil { + continue + } + cmd.AddCommand(NewCmdOfficialExtension(io, f.Prompter, em, ext)) + } + cmdutil.DisableAuthCheck(cmd) // The reference command produces paged output that displays information on every other command. diff --git a/pkg/extensions/official.go b/pkg/extensions/official.go index a1e6996db13..a07c426df91 100644 --- a/pkg/extensions/official.go +++ b/pkg/extensions/official.go @@ -12,29 +12,15 @@ type OfficialExtension struct { Repo string } -// Repository returns a ghrepo.Interface pinned to github.com for use with -// ExtensionManager.Install. +// Repository returns a ghrepo.Interface pinned to github.com so that GHES +// users install from github.com rather than their enterprise host. func (e *OfficialExtension) Repository() ghrepo.Interface { return ghrepo.NewWithHost(e.Owner, e.Repo, "github.com") } -// officialExtensions is the hard-coded registry of GitHub-owned extensions -// that gh will suggest installing when the user invokes an unknown command -// matching one of their names. -// Install suggestions include the "github.com/" host prefix so that GHES users -// install from github.com rather than their enterprise host. -var officialExtensions = []OfficialExtension{ +// OfficialExtensions is the registry of GitHub-owned extensions that gh will +// offer to install when the user invokes the corresponding command name. +var OfficialExtensions = []OfficialExtension{ {Name: "aw", Owner: "github", Repo: "gh-aw"}, {Name: "stack", Owner: "github", Repo: "gh-stack"}, } - -// FindOfficialExtension returns the matching official extension for -// commandName, or nil if none matches. -func FindOfficialExtension(commandName string) *OfficialExtension { - for _, ext := range officialExtensions { - if ext.Name == commandName { - return &ext - } - } - return nil -} diff --git a/pkg/extensions/official_test.go b/pkg/extensions/official_test.go index 0a0b5ec52ea..047af580af1 100644 --- a/pkg/extensions/official_test.go +++ b/pkg/extensions/official_test.go @@ -4,34 +4,8 @@ import ( "testing" "github.com/stretchr/testify/assert" - "github.com/stretchr/testify/require" ) -func TestFindOfficialExtension(t *testing.T) { - tests := []struct { - name string - commandName string - wantNil bool - wantRepo string - }{ - {name: "found", commandName: "stack", wantNil: false, wantRepo: "gh-stack"}, - {name: "not found", commandName: "xyzzy", wantNil: true}, - {name: "empty", commandName: "", wantNil: true}, - } - - for _, tt := range tests { - t.Run(tt.name, func(t *testing.T) { - ext := FindOfficialExtension(tt.commandName) - if tt.wantNil { - assert.Nil(t, ext) - } else { - require.NotNil(t, ext) - assert.Equal(t, tt.wantRepo, ext.Repo) - } - }) - } -} - func TestOfficialExtension_Repository(t *testing.T) { ext := &OfficialExtension{Name: "stack", Owner: "github", Repo: "gh-stack"} repo := ext.Repository() From 9596f99e565fa76a087f00c605bf93963ffb27d6 Mon Sep 17 00:00:00 2001 From: William Martin Date: Thu, 16 Apr 2026 18:09:16 +0200 Subject: [PATCH 05/12] Add no em dash rule to AGENTS.md Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- AGENTS.md | 1 + 1 file changed, 1 insertion(+) diff --git a/AGENTS.md b/AGENTS.md index b04e6b77557..a9e3ab10951 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -135,6 +135,7 @@ for _, tt := range tests { - Add godoc comments to all exported functions, types, and constants - Avoid unnecessary code comments — only comment when the *why* isn't obvious from the code - Do not comment just to restate what the code does +- Never use em dashes (—) in code, comments, or documentation; use regular dashes (-) or rewrite the sentence instead ## Error Handling From abc2a50b4c00ea1ef93907214b909f16306c9308 Mon Sep 17 00:00:00 2001 From: William Martin Date: Thu, 16 Apr 2026 18:11:28 +0200 Subject: [PATCH 06/12] Address PR review feedback - Use cmdutil.DisableAuthCheck instead of raw annotation map - Convert tests to table-driven format - Use generic extension name in tests (gh-cool) Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- pkg/cmd/root/official_extension.go | 10 +- pkg/cmd/root/official_extension_test.go | 243 +++++++++++------------- 2 files changed, 118 insertions(+), 135 deletions(-) diff --git a/pkg/cmd/root/official_extension.go b/pkg/cmd/root/official_extension.go index cde6bb6da40..bc1f21893b9 100644 --- a/pkg/cmd/root/official_extension.go +++ b/pkg/cmd/root/official_extension.go @@ -5,6 +5,7 @@ import ( "github.com/MakeNowJust/heredoc" "github.com/cli/cli/v2/internal/prompter" + "github.com/cli/cli/v2/pkg/cmdutil" "github.com/cli/cli/v2/pkg/extensions" "github.com/cli/cli/v2/pkg/iostreams" "github.com/spf13/cobra" @@ -16,14 +17,11 @@ import ( // immediately. After a successful install, the extension is dispatched with // the original arguments. func NewCmdOfficialExtension(io *iostreams.IOStreams, p prompter.Prompter, em extensions.ExtensionManager, ext *extensions.OfficialExtension) *cobra.Command { - return &cobra.Command{ + cmd := &cobra.Command{ Use: ext.Name, Short: fmt.Sprintf("Install the official %s extension", ext.Name), Hidden: true, GroupID: "extension", - Annotations: map[string]string{ - "skipAuthCheck": "true", - }, // Accept any args/flags the user may have passed so we don't get // cobra validation errors before reaching RunE. DisableFlagParsing: true, @@ -31,6 +29,10 @@ func NewCmdOfficialExtension(io *iostreams.IOStreams, p prompter.Prompter, em ex return officialExtensionRun(io, p, em, ext, args) }, } + + cmdutil.DisableAuthCheck(cmd) + + return cmd } func officialExtensionRun(io *iostreams.IOStreams, p prompter.Prompter, em extensions.ExtensionManager, ext *extensions.OfficialExtension, args []string) error { diff --git a/pkg/cmd/root/official_extension_test.go b/pkg/cmd/root/official_extension_test.go index 8b6aaa3ae14..6986c4cb5b1 100644 --- a/pkg/cmd/root/official_extension_test.go +++ b/pkg/cmd/root/official_extension_test.go @@ -13,156 +13,137 @@ import ( "github.com/stretchr/testify/require" ) -func TestOfficialExtensionRun_NonTTY(t *testing.T) { - ios, _, _, stderr := iostreams.Test() - // non-TTY by default - - ext := &extensions.OfficialExtension{Name: "stack", Owner: "github", Repo: "gh-stack"} - em := &extensions.ExtensionManagerMock{} - p := &prompter.PrompterMock{} - - err := officialExtensionRun(ios, p, em, ext, nil) - require.NoError(t, err) - - assert.Contains(t, stderr.String(), "gh stack") - assert.Contains(t, stderr.String(), "gh extension install github/gh-stack") -} - -func TestOfficialExtensionRun_TTY_Confirmed(t *testing.T) { - ios, _, _, stderr := iostreams.Test() - ios.SetStdinTTY(true) - ios.SetStdoutTTY(true) - ios.SetStderrTTY(true) - - ext := &extensions.OfficialExtension{Name: "stack", Owner: "github", Repo: "gh-stack"} - var installedRepo ghrepo.Interface - var dispatchedArgs []string - em := &extensions.ExtensionManagerMock{ - InstallFunc: func(repo ghrepo.Interface, pin string) error { - installedRepo = repo - return nil +func TestOfficialExtensionRun(t *testing.T) { + ext := &extensions.OfficialExtension{Name: "cool", Owner: "github", Repo: "gh-cool"} + + tests := []struct { + name string + isTTY bool + confirmResult bool + confirmErr error + installErr error + dispatchErr error + args []string + wantErr string + wantStderr string + wantInstalled bool + wantDispatched bool + wantDispArgs []string + }{ + { + name: "non-TTY prints install instructions", + isTTY: false, + wantStderr: "gh extension install github/gh-cool", }, - DispatchFunc: func(args []string, stdin io.Reader, stdout, stderr io.Writer) (bool, error) { - dispatchedArgs = args - return true, nil + { + name: "TTY confirmed installs and dispatches", + isTTY: true, + confirmResult: true, + args: []string{"--help"}, + wantStderr: "Successfully installed github/gh-cool", + wantInstalled: true, + wantDispatched: true, + wantDispArgs: []string{"cool", "--help"}, }, - } - p := &prompter.PrompterMock{ - ConfirmFunc: func(_ string, _ bool) (bool, error) { - return true, nil + { + name: "TTY declined does not install", + isTTY: true, + confirmResult: false, }, - } - - err := officialExtensionRun(ios, p, em, ext, []string{"--help"}) - require.NoError(t, err) - - require.NotNil(t, installedRepo) - assert.Equal(t, "github", installedRepo.RepoOwner()) - assert.Equal(t, "gh-stack", installedRepo.RepoName()) - assert.Equal(t, "github.com", installedRepo.RepoHost()) - assert.Contains(t, stderr.String(), "Successfully installed github/gh-stack") - assert.Equal(t, []string{"stack", "--help"}, dispatchedArgs) -} - -func TestOfficialExtensionRun_TTY_Declined(t *testing.T) { - ios, _, _, _ := iostreams.Test() - ios.SetStdinTTY(true) - ios.SetStdoutTTY(true) - ios.SetStderrTTY(true) - - ext := &extensions.OfficialExtension{Name: "stack", Owner: "github", Repo: "gh-stack"} - em := &extensions.ExtensionManagerMock{} - p := &prompter.PrompterMock{ - ConfirmFunc: func(_ string, _ bool) (bool, error) { - return false, nil - }, - } - - err := officialExtensionRun(ios, p, em, ext, nil) - require.NoError(t, err) - - assert.Empty(t, em.InstallCalls()) -} - -func TestOfficialExtensionRun_TTY_PromptError(t *testing.T) { - ios, _, _, _ := iostreams.Test() - ios.SetStdinTTY(true) - ios.SetStdoutTTY(true) - ios.SetStderrTTY(true) - - ext := &extensions.OfficialExtension{Name: "stack", Owner: "github", Repo: "gh-stack"} - em := &extensions.ExtensionManagerMock{} - p := &prompter.PrompterMock{ - ConfirmFunc: func(_ string, _ bool) (bool, error) { - return false, fmt.Errorf("prompt interrupted") + { + name: "TTY prompt error is propagated", + isTTY: true, + confirmErr: fmt.Errorf("prompt interrupted"), + wantErr: "prompt interrupted", }, - } - - err := officialExtensionRun(ios, p, em, ext, nil) - require.Error(t, err) - assert.Contains(t, err.Error(), "prompt interrupted") -} - -func TestOfficialExtensionRun_TTY_InstallError(t *testing.T) { - ios, _, _, _ := iostreams.Test() - ios.SetStdinTTY(true) - ios.SetStdoutTTY(true) - ios.SetStderrTTY(true) - - ext := &extensions.OfficialExtension{Name: "stack", Owner: "github", Repo: "gh-stack"} - em := &extensions.ExtensionManagerMock{ - InstallFunc: func(_ ghrepo.Interface, _ string) error { - return fmt.Errorf("network error") + { + name: "TTY install error is propagated", + isTTY: true, + confirmResult: true, + installErr: fmt.Errorf("network error"), + wantErr: "network error", + wantInstalled: true, }, - } - p := &prompter.PrompterMock{ - ConfirmFunc: func(_ string, _ bool) (bool, error) { - return true, nil + { + name: "TTY dispatch error is propagated", + isTTY: true, + confirmResult: true, + dispatchErr: fmt.Errorf("dispatch failed"), + wantErr: "dispatch failed", + wantInstalled: true, + wantDispatched: true, }, } - err := officialExtensionRun(ios, p, em, ext, nil) - require.Error(t, err) - assert.Contains(t, err.Error(), "network error") -} - -func TestOfficialExtensionRun_TTY_DispatchError(t *testing.T) { - ios, _, _, _ := iostreams.Test() - ios.SetStdinTTY(true) - ios.SetStdoutTTY(true) - ios.SetStderrTTY(true) - - ext := &extensions.OfficialExtension{Name: "stack", Owner: "github", Repo: "gh-stack"} - em := &extensions.ExtensionManagerMock{ - InstallFunc: func(_ ghrepo.Interface, _ string) error { - return nil - }, - DispatchFunc: func(_ []string, _ io.Reader, _, _ io.Writer) (bool, error) { - return false, fmt.Errorf("dispatch failed") - }, + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + ios, _, _, stderr := iostreams.Test() + if tt.isTTY { + ios.SetStdinTTY(true) + ios.SetStdoutTTY(true) + ios.SetStderrTTY(true) + } + + em := &extensions.ExtensionManagerMock{ + InstallFunc: func(_ ghrepo.Interface, _ string) error { + return tt.installErr + }, + DispatchFunc: func(_ []string, _ io.Reader, _, _ io.Writer) (bool, error) { + if tt.dispatchErr != nil { + return false, tt.dispatchErr + } + return true, nil + }, + } + p := &prompter.PrompterMock{ + ConfirmFunc: func(_ string, _ bool) (bool, error) { + return tt.confirmResult, tt.confirmErr + }, + } + + err := officialExtensionRun(ios, p, em, ext, tt.args) + + if tt.wantErr != "" { + require.Error(t, err) + assert.Contains(t, err.Error(), tt.wantErr) + } else { + require.NoError(t, err) + } + + if tt.wantStderr != "" { + assert.Contains(t, stderr.String(), tt.wantStderr) + } + + if tt.wantInstalled { + require.NotEmpty(t, em.InstallCalls()) + repo := em.InstallCalls()[0].InterfaceMoqParam + assert.Equal(t, "github", repo.RepoOwner()) + assert.Equal(t, "gh-cool", repo.RepoName()) + assert.Equal(t, "github.com", repo.RepoHost()) + } else if tt.isTTY && !tt.confirmResult && tt.confirmErr == nil { + assert.Empty(t, em.InstallCalls()) + } + + if tt.wantDispatched { + require.NotEmpty(t, em.DispatchCalls()) + if tt.wantDispArgs != nil { + assert.Equal(t, tt.wantDispArgs, em.DispatchCalls()[0].Args) + } + } + }) } - p := &prompter.PrompterMock{ - ConfirmFunc: func(_ string, _ bool) (bool, error) { - return true, nil - }, - } - - err := officialExtensionRun(ios, p, em, ext, nil) - require.Error(t, err) - assert.Contains(t, err.Error(), "dispatch failed") } func TestNewCmdOfficialExtension_Properties(t *testing.T) { ios, _, _, _ := iostreams.Test() - ext := &extensions.OfficialExtension{Name: "stack", Owner: "github", Repo: "gh-stack"} + ext := &extensions.OfficialExtension{Name: "cool", Owner: "github", Repo: "gh-cool"} em := &extensions.ExtensionManagerMock{} p := &prompter.PrompterMock{} cmd := NewCmdOfficialExtension(ios, p, em, ext) - assert.Equal(t, "stack", cmd.Use) + assert.Equal(t, "cool", cmd.Use) assert.True(t, cmd.Hidden) assert.Equal(t, "extension", cmd.GroupID) assert.True(t, cmd.DisableFlagParsing) - assert.Equal(t, "true", cmd.Annotations["skipAuthCheck"]) } From 1d59334964454df2e3a6360c76e322af24b69d1b Mon Sep 17 00:00:00 2001 From: Kynan Ware <47394200+BagToad@users.noreply.github.com> Date: Thu, 16 Apr 2026 10:30:47 -0600 Subject: [PATCH 07/12] Replace em-dashes with regular dashes Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- pkg/cmd/root/root.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/pkg/cmd/root/root.go b/pkg/cmd/root/root.go index 793b3bc8320..700ca570dd1 100644 --- a/pkg/cmd/root/root.go +++ b/pkg/cmd/root/root.go @@ -230,7 +230,7 @@ func NewCmdRoot(f *cmdutil.Factory, version, buildDate string) (*cobra.Command, } } - // Official extension stubs — hidden commands that suggest installing + // Official extension stubs: hidden commands that suggest installing // GitHub-owned extensions when invoked. Registered after real extensions // and aliases so that both take priority over stubs. for i := range extensions.OfficialExtensions { From b8d504cbd9de92825440ce7b130467eba2b06698 Mon Sep 17 00:00:00 2001 From: Kynan Ware <47394200+BagToad@users.noreply.github.com> Date: Thu, 16 Apr 2026 10:34:48 -0600 Subject: [PATCH 08/12] Rename official extension files and types to stubs Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- .../{official_extension.go => official_extension_stub.go} | 8 ++++---- ..._extension_test.go => official_extension_stub_test.go} | 8 ++++---- pkg/cmd/root/root.go | 2 +- 3 files changed, 9 insertions(+), 9 deletions(-) rename pkg/cmd/root/{official_extension.go => official_extension_stub.go} (81%) rename pkg/cmd/root/{official_extension_test.go => official_extension_stub_test.go} (94%) diff --git a/pkg/cmd/root/official_extension.go b/pkg/cmd/root/official_extension_stub.go similarity index 81% rename from pkg/cmd/root/official_extension.go rename to pkg/cmd/root/official_extension_stub.go index bc1f21893b9..2e5a69bec41 100644 --- a/pkg/cmd/root/official_extension.go +++ b/pkg/cmd/root/official_extension_stub.go @@ -11,12 +11,12 @@ import ( "github.com/spf13/cobra" ) -// NewCmdOfficialExtension creates a hidden stub command for an official +// NewCmdOfficialExtensionStub creates a hidden stub command for an official // extension that has not yet been installed. When invoked, it suggests // installing the extension and, in interactive sessions, offers to do so // immediately. After a successful install, the extension is dispatched with // the original arguments. -func NewCmdOfficialExtension(io *iostreams.IOStreams, p prompter.Prompter, em extensions.ExtensionManager, ext *extensions.OfficialExtension) *cobra.Command { +func NewCmdOfficialExtensionStub(io *iostreams.IOStreams, p prompter.Prompter, em extensions.ExtensionManager, ext *extensions.OfficialExtension) *cobra.Command { cmd := &cobra.Command{ Use: ext.Name, Short: fmt.Sprintf("Install the official %s extension", ext.Name), @@ -26,7 +26,7 @@ func NewCmdOfficialExtension(io *iostreams.IOStreams, p prompter.Prompter, em ex // cobra validation errors before reaching RunE. DisableFlagParsing: true, RunE: func(cmd *cobra.Command, args []string) error { - return officialExtensionRun(io, p, em, ext, args) + return officialExtensionStubRun(io, p, em, ext, args) }, } @@ -35,7 +35,7 @@ func NewCmdOfficialExtension(io *iostreams.IOStreams, p prompter.Prompter, em ex return cmd } -func officialExtensionRun(io *iostreams.IOStreams, p prompter.Prompter, em extensions.ExtensionManager, ext *extensions.OfficialExtension, args []string) error { +func officialExtensionStubRun(io *iostreams.IOStreams, p prompter.Prompter, em extensions.ExtensionManager, ext *extensions.OfficialExtension, args []string) error { stderr := io.ErrOut if !io.CanPrompt() { diff --git a/pkg/cmd/root/official_extension_test.go b/pkg/cmd/root/official_extension_stub_test.go similarity index 94% rename from pkg/cmd/root/official_extension_test.go rename to pkg/cmd/root/official_extension_stub_test.go index 6986c4cb5b1..4fadd67d318 100644 --- a/pkg/cmd/root/official_extension_test.go +++ b/pkg/cmd/root/official_extension_stub_test.go @@ -13,7 +13,7 @@ import ( "github.com/stretchr/testify/require" ) -func TestOfficialExtensionRun(t *testing.T) { +func TestOfficialExtensionStubRun(t *testing.T) { ext := &extensions.OfficialExtension{Name: "cool", Owner: "github", Repo: "gh-cool"} tests := []struct { @@ -101,7 +101,7 @@ func TestOfficialExtensionRun(t *testing.T) { }, } - err := officialExtensionRun(ios, p, em, ext, tt.args) + err := officialExtensionStubRun(ios, p, em, ext, tt.args) if tt.wantErr != "" { require.Error(t, err) @@ -134,13 +134,13 @@ func TestOfficialExtensionRun(t *testing.T) { } } -func TestNewCmdOfficialExtension_Properties(t *testing.T) { +func TestNewCmdOfficialExtensionStub_Properties(t *testing.T) { ios, _, _, _ := iostreams.Test() ext := &extensions.OfficialExtension{Name: "cool", Owner: "github", Repo: "gh-cool"} em := &extensions.ExtensionManagerMock{} p := &prompter.PrompterMock{} - cmd := NewCmdOfficialExtension(ios, p, em, ext) + cmd := NewCmdOfficialExtensionStub(ios, p, em, ext) assert.Equal(t, "cool", cmd.Use) assert.True(t, cmd.Hidden) diff --git a/pkg/cmd/root/root.go b/pkg/cmd/root/root.go index 700ca570dd1..068129ba23f 100644 --- a/pkg/cmd/root/root.go +++ b/pkg/cmd/root/root.go @@ -238,7 +238,7 @@ func NewCmdRoot(f *cmdutil.Factory, version, buildDate string) (*cobra.Command, if _, _, err := cmd.Find([]string{ext.Name}); err == nil { continue } - cmd.AddCommand(NewCmdOfficialExtension(io, f.Prompter, em, ext)) + cmd.AddCommand(NewCmdOfficialExtensionStub(io, f.Prompter, em, ext)) } cmdutil.DisableAuthCheck(cmd) From 32b8af1017ae39c605aed85d7b13acff0721939b Mon Sep 17 00:00:00 2001 From: Kynan Ware <47394200+BagToad@users.noreply.github.com> Date: Thu, 16 Apr 2026 10:38:33 -0600 Subject: [PATCH 09/12] Remove dispatch after install, install only Removes post-install extension dispatch to keep the stub focused on installation. Also removes unused args parameter from the run function. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- pkg/cmd/root/official_extension_stub.go | 10 +--- pkg/cmd/root/official_extension_stub_test.go | 58 +++++--------------- 2 files changed, 16 insertions(+), 52 deletions(-) diff --git a/pkg/cmd/root/official_extension_stub.go b/pkg/cmd/root/official_extension_stub.go index 2e5a69bec41..af52e43663e 100644 --- a/pkg/cmd/root/official_extension_stub.go +++ b/pkg/cmd/root/official_extension_stub.go @@ -26,7 +26,7 @@ func NewCmdOfficialExtensionStub(io *iostreams.IOStreams, p prompter.Prompter, e // cobra validation errors before reaching RunE. DisableFlagParsing: true, RunE: func(cmd *cobra.Command, args []string) error { - return officialExtensionStubRun(io, p, em, ext, args) + return officialExtensionStubRun(io, p, em, ext) }, } @@ -35,7 +35,7 @@ func NewCmdOfficialExtensionStub(io *iostreams.IOStreams, p prompter.Prompter, e return cmd } -func officialExtensionStubRun(io *iostreams.IOStreams, p prompter.Prompter, em extensions.ExtensionManager, ext *extensions.OfficialExtension, args []string) error { +func officialExtensionStubRun(io *iostreams.IOStreams, p prompter.Prompter, em extensions.ExtensionManager, ext *extensions.OfficialExtension) error { stderr := io.ErrOut if !io.CanPrompt() { @@ -68,11 +68,5 @@ func officialExtensionStubRun(io *iostreams.IOStreams, p prompter.Prompter, em e } fmt.Fprintf(stderr, "Successfully installed %s/%s\n", ext.Owner, ext.Repo) - - // Dispatch the newly installed extension with the original arguments. - dispatchArgs := append([]string{ext.Name}, args...) - if _, dispatchErr := em.Dispatch(dispatchArgs, io.In, io.Out, stderr); dispatchErr != nil { - return dispatchErr - } return nil } diff --git a/pkg/cmd/root/official_extension_stub_test.go b/pkg/cmd/root/official_extension_stub_test.go index 4fadd67d318..d2fd4624204 100644 --- a/pkg/cmd/root/official_extension_stub_test.go +++ b/pkg/cmd/root/official_extension_stub_test.go @@ -2,7 +2,6 @@ package root import ( "fmt" - "io" "testing" "github.com/cli/cli/v2/internal/ghrepo" @@ -17,18 +16,14 @@ func TestOfficialExtensionStubRun(t *testing.T) { ext := &extensions.OfficialExtension{Name: "cool", Owner: "github", Repo: "gh-cool"} tests := []struct { - name string - isTTY bool - confirmResult bool - confirmErr error - installErr error - dispatchErr error - args []string - wantErr string - wantStderr string - wantInstalled bool - wantDispatched bool - wantDispArgs []string + name string + isTTY bool + confirmResult bool + confirmErr error + installErr error + wantErr string + wantStderr string + wantInstalled bool }{ { name: "non-TTY prints install instructions", @@ -36,14 +31,11 @@ func TestOfficialExtensionStubRun(t *testing.T) { wantStderr: "gh extension install github/gh-cool", }, { - name: "TTY confirmed installs and dispatches", - isTTY: true, - confirmResult: true, - args: []string{"--help"}, - wantStderr: "Successfully installed github/gh-cool", - wantInstalled: true, - wantDispatched: true, - wantDispArgs: []string{"cool", "--help"}, + name: "TTY confirmed installs", + isTTY: true, + confirmResult: true, + wantStderr: "Successfully installed github/gh-cool", + wantInstalled: true, }, { name: "TTY declined does not install", @@ -64,15 +56,6 @@ func TestOfficialExtensionStubRun(t *testing.T) { wantErr: "network error", wantInstalled: true, }, - { - name: "TTY dispatch error is propagated", - isTTY: true, - confirmResult: true, - dispatchErr: fmt.Errorf("dispatch failed"), - wantErr: "dispatch failed", - wantInstalled: true, - wantDispatched: true, - }, } for _, tt := range tests { @@ -88,12 +71,6 @@ func TestOfficialExtensionStubRun(t *testing.T) { InstallFunc: func(_ ghrepo.Interface, _ string) error { return tt.installErr }, - DispatchFunc: func(_ []string, _ io.Reader, _, _ io.Writer) (bool, error) { - if tt.dispatchErr != nil { - return false, tt.dispatchErr - } - return true, nil - }, } p := &prompter.PrompterMock{ ConfirmFunc: func(_ string, _ bool) (bool, error) { @@ -101,7 +78,7 @@ func TestOfficialExtensionStubRun(t *testing.T) { }, } - err := officialExtensionStubRun(ios, p, em, ext, tt.args) + err := officialExtensionStubRun(ios, p, em, ext) if tt.wantErr != "" { require.Error(t, err) @@ -123,13 +100,6 @@ func TestOfficialExtensionStubRun(t *testing.T) { } else if tt.isTTY && !tt.confirmResult && tt.confirmErr == nil { assert.Empty(t, em.InstallCalls()) } - - if tt.wantDispatched { - require.NotEmpty(t, em.DispatchCalls()) - if tt.wantDispArgs != nil { - assert.Equal(t, tt.wantDispArgs, em.DispatchCalls()[0].Args) - } - } }) } } From de204f85186f29189fd4ec584bd1d2fcc57f890e Mon Sep 17 00:00:00 2001 From: Sam Morrow Date: Thu, 16 Apr 2026 18:49:54 +0200 Subject: [PATCH 10/12] =?UTF-8?q?fix:=20apply=20review=20feedback=20?= =?UTF-8?q?=E2=80=94=20nil=20HttpClient,=20local=20dedup=20type?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - Return nil instead of real http.Client in unsupported host test - Move skillResultKey type inside deduplicateResults function scope Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- pkg/cmd/skills/publish/publish_test.go | 2 +- pkg/cmd/skills/search/search.go | 16 +++++++--------- 2 files changed, 8 insertions(+), 10 deletions(-) diff --git a/pkg/cmd/skills/publish/publish_test.go b/pkg/cmd/skills/publish/publish_test.go index 04f400733ef..97795a61712 100644 --- a/pkg/cmd/skills/publish/publish_test.go +++ b/pkg/cmd/skills/publish/publish_test.go @@ -164,7 +164,7 @@ func TestPublishRun_UnsupportedHost(t *testing.T) { IO: ios, Dir: dir, GitClient: &git.Client{}, - HttpClient: func() (*http.Client, error) { return &http.Client{}, nil }, + HttpClient: func() (*http.Client, error) { return nil, nil }, host: "acme.ghes.com", }) require.ErrorContains(t, err, "supports only github.com") diff --git a/pkg/cmd/skills/search/search.go b/pkg/cmd/skills/search/search.go index 18471cc954f..05511484eae 100644 --- a/pkg/cmd/skills/search/search.go +++ b/pkg/cmd/skills/search/search.go @@ -770,17 +770,15 @@ func fetchPrimaryPages(client *api.Client, host, query string, displayPage, disp return allItems, totalCount, nil } -// skillResultKey is a typed map key for deduplicating code search results -// by (repo, namespace, skill name). All fields are lowercased for -// case-insensitive comparison. -type skillResultKey struct { - repo string - namespace string - skillName string -} - // deduplicateResults extracts unique (repo, namespace, skill name) triples from code search hits. func deduplicateResults(items []codeSearchItem) []skillResult { + // skillResultKey is a typed map key that deduplicates by (repo, namespace, + // skill name). All fields are lowercased for case-insensitive comparison. + type skillResultKey struct { + repo string + namespace string + skillName string + } seen := make(map[skillResultKey]struct{}) var results []skillResult From 5ae5d1db7f7f09416b2aa5f00b42ef6cc8c8034a Mon Sep 17 00:00:00 2001 From: Copilot <223556219+Copilot@users.noreply.github.com> Date: Thu, 16 Apr 2026 19:02:53 +0200 Subject: [PATCH 11/12] refactor: replace real git with run.CommandStubber in publish tests Replace all exec.Command("git", ...), initGitRepo, runGitInDir, and newTestGitClientWithUpstream with run.Stub()/run.CommandStubber stubs. Changes: - Remove os/exec and strings imports; add fmt, regexp, internal/run - Add newTestGitClient(), stubGitRemote(), stubEnsurePushed() helpers - Remove initGitRepo, runGitInDir, newTestGitClientWithUpstream helpers - Add cmdStubs field to TestPublishRun table struct - Convert all test cases to use stub-based git interactions - Use regexp.QuoteMeta for remote name patterns - Use %[1]s/%[2]s format args in stubGitRemote - Initialize git.Client with explicit GitPath to avoid real git resolution - Rewrite TestEnsurePushed with stub-based tests - Update TestDetectGitHubRemote_UsesDir and TestPublishRun_DirArgUsesTargetRemote Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- pkg/cmd/skills/publish/publish_test.go | 461 ++++++++++++------------- 1 file changed, 217 insertions(+), 244 deletions(-) diff --git a/pkg/cmd/skills/publish/publish_test.go b/pkg/cmd/skills/publish/publish_test.go index 97795a61712..29142d44f47 100644 --- a/pkg/cmd/skills/publish/publish_test.go +++ b/pkg/cmd/skills/publish/publish_test.go @@ -2,16 +2,17 @@ package publish import ( "bytes" + "fmt" "net/http" "os" - "os/exec" "path/filepath" - "strings" + "regexp" "testing" "github.com/MakeNowJust/heredoc" "github.com/cli/cli/v2/git" "github.com/cli/cli/v2/internal/prompter" + "github.com/cli/cli/v2/internal/run" "github.com/cli/cli/v2/pkg/cmdutil" "github.com/cli/cli/v2/pkg/httpmock" "github.com/cli/cli/v2/pkg/iostreams" @@ -20,33 +21,31 @@ import ( "github.com/stretchr/testify/require" ) -// initGitRepo initializes a git repo in the given directory and adds remotes. -// Use this when the git repo must live in the same directory as the skill files. -// A local bare repo is created as the push target so that ensurePushed can work -// during publish tests, while the fetch URL remains the GitHub URL so that -// detectGitHubRemote still resolves the correct owner/repo. -func initGitRepo(t *testing.T, dir string, remoteURLs map[string]string) { - t.Helper() - - bareDir := filepath.Join(t.TempDir(), "upstream.git") - require.NoError(t, os.MkdirAll(bareDir, 0o755)) - runGitInDir(t, bareDir, "init", "--bare", "--initial-branch=main") +// newTestGitClient returns a git.Client with a fake git path to avoid real git resolution. +func newTestGitClient() *git.Client { + return &git.Client{GitPath: "some/path/git"} +} - runGitInDir(t, dir, "init", "--initial-branch=main") - runGitInDir(t, dir, "config", "user.email", "monalisa@github.com") - runGitInDir(t, dir, "config", "user.name", "Monalisa Octocat") +// stubGitRemote registers CommandStubber stubs for git remote detection. +func stubGitRemote(cs *run.CommandStubber, remoteURLs map[string]string) { + var remoteLines string for name, url := range remoteURLs { - runGitInDir(t, dir, "remote", "add", name, url) - runGitInDir(t, dir, "remote", "set-url", "--push", name, bareDir) + remoteLines += fmt.Sprintf("%[1]s\t%[2]s (fetch)\n%[1]s\t%[2]s (push)\n", name, url) } - - runGitInDir(t, dir, "add", ".") - runGitInDir(t, dir, "commit", "--allow-empty", "-m", "init") - if _, ok := remoteURLs["origin"]; ok { - runGitInDir(t, dir, "push", "origin", "main") + cs.Register(`git( .+)? remote -v`, 0, remoteLines) + cs.Register(`git( .+)? config --get-regexp \^remote\\\.`, 1, "") + for name, url := range remoteURLs { + cs.Register(fmt.Sprintf(`git( .+)? remote get-url -- %s`, regexp.QuoteMeta(name)), 0, url+"\n") } } +// stubEnsurePushed registers stubs for ensurePushed + runPublishRelease CurrentBranch calls. +func stubEnsurePushed(cs *run.CommandStubber, branch string) { + cs.Register(`git( .+)? symbolic-ref --quiet HEAD`, 0, "refs/heads/"+branch+"\n") + cs.Register(`git( .+)? rev-list --count @\{push\}\.\.HEAD`, 0, "0\n") + cs.Register(`git( .+)? symbolic-ref --quiet HEAD`, 0, "refs/heads/"+branch+"\n") +} + // stubAllSecureRemote registers the standard stubs for a fully-configured remote // repo (topics, tags, rulesets, security) so publishRun skips all remote warnings. func stubAllSecureRemote(reg *httpmock.Registry, owner, repo string) { @@ -158,12 +157,15 @@ func TestPublishRun_UnsupportedHost(t *testing.T) { Body. `)) + cs, cmdTeardown := run.Stub() + defer cmdTeardown(t) + stubGitRemote(cs, map[string]string{"origin": "https://github.com/monalisa/skills-repo.git"}) + ios, _, _, _ := iostreams.Test() - initGitRepo(t, dir, map[string]string{"origin": "https://github.com/monalisa/skills-repo.git"}) err := publishRun(&PublishOptions{ IO: ios, Dir: dir, - GitClient: &git.Client{}, + GitClient: newTestGitClient(), HttpClient: func() (*http.Client, error) { return nil, nil }, host: "acme.ghes.com", }) @@ -176,6 +178,7 @@ func TestPublishRun(t *testing.T) { isTTY bool setup func(t *testing.T, dir string) stubs func(*httpmock.Registry) + cmdStubs func(*run.CommandStubber) opts func(ios *iostreams.IOStreams, dir string, reg *httpmock.Registry) *PublishOptions verify func(t *testing.T, dir string) wantErr string @@ -275,20 +278,22 @@ func TestPublishRun(t *testing.T) { --- Body. `)), 0o644)) - initGitRepo(t, dir, map[string]string{ - "origin": "https://github.com/monalisa/skills-repo.git", - }) }, stubs: func(reg *httpmock.Registry) { stubAllSecureRemote(reg, "monalisa", "skills-repo") }, + cmdStubs: func(cs *run.CommandStubber) { + stubGitRemote(cs, map[string]string{ + "origin": "https://github.com/monalisa/skills-repo.git", + }) + }, opts: func(ios *iostreams.IOStreams, dir string, reg *httpmock.Registry) *PublishOptions { t.Helper() return &PublishOptions{ IO: ios, Dir: dir, DryRun: true, - GitClient: &git.Client{}, + GitClient: newTestGitClient(), HttpClient: func() (*http.Client, error) { return &http.Client{Transport: reg}, nil }, host: "github.com", } @@ -312,20 +317,22 @@ func TestPublishRun(t *testing.T) { --- Body. `)), 0o644)) - initGitRepo(t, dir, map[string]string{ - "origin": "https://github.com/monalisa/skills-repo.git", - }) }, stubs: func(reg *httpmock.Registry) { stubAllSecureRemote(reg, "monalisa", "skills-repo") }, + cmdStubs: func(cs *run.CommandStubber) { + stubGitRemote(cs, map[string]string{ + "origin": "https://github.com/monalisa/skills-repo.git", + }) + }, opts: func(ios *iostreams.IOStreams, dir string, reg *httpmock.Registry) *PublishOptions { t.Helper() return &PublishOptions{ IO: ios, Dir: dir, DryRun: true, - GitClient: &git.Client{}, + GitClient: newTestGitClient(), HttpClient: func() (*http.Client, error) { return &http.Client{Transport: reg}, nil }, host: "github.com", } @@ -350,16 +357,18 @@ 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 { - t.Helper() - initGitRepo(t, dir, map[string]string{ + cmdStubs: func(cs *run.CommandStubber) { + stubGitRemote(cs, map[string]string{ "origin": "https://github.com/monalisa/skills-repo.git", }) + }, + opts: func(ios *iostreams.IOStreams, dir string, reg *httpmock.Registry) *PublishOptions { + t.Helper() return &PublishOptions{ IO: ios, Dir: dir, DryRun: true, - GitClient: &git.Client{}, + GitClient: newTestGitClient(), HttpClient: func() (*http.Client, error) { return &http.Client{Transport: reg}, nil }, host: "github.com", } @@ -403,11 +412,14 @@ func TestPublishRun(t *testing.T) { }), ) }, - opts: func(ios *iostreams.IOStreams, dir string, reg *httpmock.Registry) *PublishOptions { - t.Helper() - initGitRepo(t, dir, map[string]string{ + cmdStubs: func(cs *run.CommandStubber) { + stubGitRemote(cs, map[string]string{ "origin": "https://github.com/monalisa/skills-repo.git", }) + stubEnsurePushed(cs, "main") + }, + opts: func(ios *iostreams.IOStreams, dir string, reg *httpmock.Registry) *PublishOptions { + t.Helper() return &PublishOptions{ IO: ios, Dir: dir, @@ -415,7 +427,7 @@ func TestPublishRun(t *testing.T) { Prompter: &prompter.PrompterMock{ ConfirmFunc: func(msg string, def bool) (bool, error) { return true, nil }, }, - GitClient: &git.Client{}, + GitClient: newTestGitClient(), HttpClient: func() (*http.Client, error) { return &http.Client{Transport: reg}, nil }, host: "github.com", } @@ -557,15 +569,17 @@ func TestPublishRun(t *testing.T) { }), ) }, - opts: func(ios *iostreams.IOStreams, dir string, reg *httpmock.Registry) *PublishOptions { - t.Helper() - initGitRepo(t, dir, map[string]string{ + cmdStubs: func(cs *run.CommandStubber) { + stubGitRemote(cs, map[string]string{ "origin": "https://github.com/octocat/secure-repo.git", }) + }, + opts: func(ios *iostreams.IOStreams, dir string, reg *httpmock.Registry) *PublishOptions { + t.Helper() return &PublishOptions{ IO: ios, Dir: dir, - GitClient: &git.Client{}, + GitClient: newTestGitClient(), HttpClient: func() (*http.Client, error) { return &http.Client{Transport: reg}, nil }, host: "github.com", } @@ -610,15 +624,17 @@ func TestPublishRun(t *testing.T) { }), ) }, - opts: func(ios *iostreams.IOStreams, dir string, reg *httpmock.Registry) *PublishOptions { - t.Helper() - initGitRepo(t, dir, map[string]string{ + cmdStubs: func(cs *run.CommandStubber) { + stubGitRemote(cs, map[string]string{ "origin": "https://github.com/octocat/tag-repo.git", }) + }, + opts: func(ios *iostreams.IOStreams, dir string, reg *httpmock.Registry) *PublishOptions { + t.Helper() return &PublishOptions{ IO: ios, Dir: dir, - GitClient: &git.Client{}, + GitClient: newTestGitClient(), HttpClient: func() (*http.Client, error) { return &http.Client{Transport: reg}, nil }, host: "github.com", } @@ -673,16 +689,18 @@ func TestPublishRun(t *testing.T) { httpmock.StatusStringResponse(404, "not found"), ) }, - opts: func(ios *iostreams.IOStreams, dir string, reg *httpmock.Registry) *PublishOptions { - t.Helper() - initGitRepo(t, dir, map[string]string{ + cmdStubs: func(cs *run.CommandStubber) { + stubGitRemote(cs, map[string]string{ "origin": "https://github.com/octocat/code-repo.git", }) + }, + opts: func(ios *iostreams.IOStreams, dir string, reg *httpmock.Registry) *PublishOptions { + t.Helper() return &PublishOptions{ IO: ios, Dir: dir, DryRun: true, - GitClient: &git.Client{}, + GitClient: newTestGitClient(), HttpClient: func() (*http.Client, error) { return &http.Client{Transport: reg}, nil }, host: "github.com", } @@ -738,16 +756,18 @@ func TestPublishRun(t *testing.T) { httpmock.StatusStringResponse(404, "not found"), ) }, - opts: func(ios *iostreams.IOStreams, dir string, reg *httpmock.Registry) *PublishOptions { - t.Helper() - initGitRepo(t, dir, map[string]string{ + cmdStubs: func(cs *run.CommandStubber) { + stubGitRemote(cs, map[string]string{ "origin": "https://github.com/octocat/dep-repo.git", }) + }, + opts: func(ios *iostreams.IOStreams, dir string, reg *httpmock.Registry) *PublishOptions { + t.Helper() return &PublishOptions{ IO: ios, Dir: dir, DryRun: true, - GitClient: &git.Client{}, + GitClient: newTestGitClient(), HttpClient: func() (*http.Client, error) { return &http.Client{Transport: reg}, nil }, host: "github.com", } @@ -767,21 +787,25 @@ func TestPublishRun(t *testing.T) { --- Body. `)) + require.NoError(t, os.MkdirAll(filepath.Join(dir, ".agents", "skills", "installed"), 0o755)) + }, + cmdStubs: func(cs *run.CommandStubber) { + cs.Register(`git( .+)? check-ignore -q -- .agents/skills`, 1, "") + cs.Register(`git( .+)? remote -v`, 0, "") + cs.Register(`git( .+)? config --get-regexp \^remote\\\.`, 1, "") + cs.Register(`git( .+)? rev-parse --git-dir`, 0, ".git\n") + cs.Register(`git( .+)? remote -v`, 0, "") + cs.Register(`git( .+)? config --get-regexp \^remote\\\.`, 1, "") }, 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{ IO: ios, Dir: dir, - GitClient: &git.Client{RepoDir: dir}, + GitClient: &git.Client{GitPath: "some/path/git", RepoDir: dir}, } }, - wantStdout: ".gitignore", + wantStdout: "may contain installed skills that are not gitignored", }, { name: "installed skill dirs gitignored no warning", @@ -796,20 +820,21 @@ func TestPublishRun(t *testing.T) { Body. `)) 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") - require.NoError(t, os.WriteFile(filepath.Join(dir, ".gitignore"), []byte(".agents/skills\n"), 0o644)) - runGitInDir(t, dir, "add", ".gitignore") - runGitInDir(t, dir, "commit", "-m", "init") + }, + cmdStubs: func(cs *run.CommandStubber) { + cs.Register(`git( .+)? check-ignore -q -- .agents/skills`, 0, "") + cs.Register(`git( .+)? remote -v`, 0, "") + cs.Register(`git( .+)? config --get-regexp \^remote\\\.`, 1, "") + cs.Register(`git( .+)? rev-parse --git-dir`, 0, ".git\n") + cs.Register(`git( .+)? remote -v`, 0, "") + cs.Register(`git( .+)? config --get-regexp \^remote\\\.`, 1, "") }, opts: func(ios *iostreams.IOStreams, dir string, _ *httpmock.Registry) *PublishOptions { t.Helper() return &PublishOptions{ IO: ios, Dir: dir, - GitClient: &git.Client{RepoDir: dir}, + GitClient: &git.Client{GitPath: "some/path/git", RepoDir: dir}, } }, wantStdout: "no git remote", @@ -830,15 +855,22 @@ func TestPublishRun(t *testing.T) { --- 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)) }, + cmdStubs: func(cs *run.CommandStubber) { + cs.Register(`git( .+)? check-ignore -q -- .agents/skills`, 128, "") + cs.Register(`git( .+)? remote -v`, 0, "") + cs.Register(`git( .+)? config --get-regexp \^remote\\\.`, 1, "") + cs.Register(`git( .+)? rev-parse --git-dir`, 0, ".git\n") + cs.Register(`git( .+)? remote -v`, 0, "") + cs.Register(`git( .+)? config --get-regexp \^remote\\\.`, 1, "") + }, opts: func(ios *iostreams.IOStreams, dir string, _ *httpmock.Registry) *PublishOptions { t.Helper() return &PublishOptions{ IO: ios, Dir: dir, - GitClient: &git.Client{RepoDir: dir}, + GitClient: &git.Client{GitPath: "some/path/git", RepoDir: dir}, } }, wantStdout: "may contain installed skills that are not gitignored", @@ -855,17 +887,22 @@ func TestPublishRun(t *testing.T) { --- Body. `)) - runGitInDir(t, dir, "init", "--initial-branch=main") - runGitInDir(t, dir, "config", "user.email", "monalisa@github.com") - runGitInDir(t, dir, "config", "user.name", "Monalisa Octocat") - runGitInDir(t, dir, "remote", "add", "origin", "https://gitlab.com/hubot/bar.git") + }, + cmdStubs: func(cs *run.CommandStubber) { + stubGitRemote(cs, map[string]string{ + "origin": "https://gitlab.com/hubot/bar.git", + }) + cs.Register(`git( .+)? rev-parse --git-dir`, 0, ".git\n") + cs.Register(`git( .+)? remote -v`, 0, "origin\thttps://gitlab.com/hubot/bar.git (fetch)\norigin\thttps://gitlab.com/hubot/bar.git (push)\n") + cs.Register(`git( .+)? config --get-regexp \^remote\\\.`, 1, "") + cs.Register(fmt.Sprintf(`git( .+)? remote get-url -- %s`, regexp.QuoteMeta("origin")), 0, "https://gitlab.com/hubot/bar.git\n") }, opts: func(ios *iostreams.IOStreams, dir string, _ *httpmock.Registry) *PublishOptions { t.Helper() return &PublishOptions{ IO: ios, Dir: dir, - GitClient: &git.Client{RepoDir: dir}, + GitClient: &git.Client{GitPath: "some/path/git", RepoDir: dir}, } }, wantStdout: "not a GitHub repository", @@ -887,17 +924,19 @@ func TestPublishRun(t *testing.T) { stubs: func(reg *httpmock.Registry) { stubAllSecureRemote(reg, "octocat", "repo") }, + cmdStubs: func(cs *run.CommandStubber) { + cs.Register(`git( .+)? remote -v`, 0, "origin\thttps://gitlab.com/hubot/bar.git (fetch)\norigin\thttps://gitlab.com/hubot/bar.git (push)\nupstream\tgit@github.com:octocat/repo.git (fetch)\nupstream\tgit@github.com:octocat/repo.git (push)\n") + cs.Register(`git( .+)? config --get-regexp \^remote\\\.`, 1, "") + // upstream sorts first (score 3 > 1), so only upstream's get-url is called + cs.Register(fmt.Sprintf(`git( .+)? remote get-url -- %s`, regexp.QuoteMeta("upstream")), 0, "git@github.com:octocat/repo.git\n") + }, opts: func(ios *iostreams.IOStreams, dir string, reg *httpmock.Registry) *PublishOptions { t.Helper() - initGitRepo(t, dir, map[string]string{ - "origin": "https://gitlab.com/hubot/bar.git", - "upstream": "git@github.com:octocat/repo.git", - }) return &PublishOptions{ IO: ios, Dir: dir, DryRun: true, - GitClient: &git.Client{}, + GitClient: newTestGitClient(), HttpClient: func() (*http.Client, error) { return &http.Client{Transport: reg}, nil }, host: "github.com", } @@ -974,11 +1013,14 @@ func TestPublishRun(t *testing.T) { }), ) }, - opts: func(ios *iostreams.IOStreams, dir string, reg *httpmock.Registry) *PublishOptions { - t.Helper() - initGitRepo(t, dir, map[string]string{ + cmdStubs: func(cs *run.CommandStubber) { + stubGitRemote(cs, map[string]string{ "origin": "https://github.com/monalisa/skills-repo.git", }) + stubEnsurePushed(cs, "main") + }, + opts: func(ios *iostreams.IOStreams, dir string, reg *httpmock.Registry) *PublishOptions { + t.Helper() return &PublishOptions{ IO: ios, Dir: dir, @@ -986,12 +1028,11 @@ func TestPublishRun(t *testing.T) { Prompter: &prompter.PrompterMock{ ConfirmFunc: func(msg string, def bool) (bool, error) { return true, nil }, }, - GitClient: &git.Client{}, + GitClient: newTestGitClient(), HttpClient: func() (*http.Client, error) { return &http.Client{Transport: reg}, nil }, host: "github.com", } }, - wantStdout: "Added \"agent-skills\" topic", }, { name: "tag suggestion uses existing tags", @@ -1052,16 +1093,19 @@ func TestPublishRun(t *testing.T) { }), ) }, - opts: func(ios *iostreams.IOStreams, dir string, reg *httpmock.Registry) *PublishOptions { - t.Helper() - initGitRepo(t, dir, map[string]string{ + cmdStubs: func(cs *run.CommandStubber) { + stubGitRemote(cs, map[string]string{ "origin": "https://github.com/monalisa/skills-repo.git", }) + stubEnsurePushed(cs, "main") + }, + opts: func(ios *iostreams.IOStreams, dir string, reg *httpmock.Registry) *PublishOptions { + t.Helper() return &PublishOptions{ IO: ios, Dir: dir, Tag: "v2.3.5", - GitClient: &git.Client{}, + GitClient: newTestGitClient(), HttpClient: func() (*http.Client, error) { return &http.Client{Transport: reg}, nil }, host: "github.com", } @@ -1084,16 +1128,20 @@ 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 { - t.Helper() - initGitRepo(t, dir, map[string]string{ + cmdStubs: func(cs *run.CommandStubber) { + stubGitRemote(cs, map[string]string{ "origin": "https://github.com/monalisa/skills-repo.git", }) + cs.Register(`git( .+)? symbolic-ref --quiet HEAD`, 0, "refs/heads/main\n") + cs.Register(`git( .+)? rev-list --count @\{push\}\.\.HEAD`, 0, "0\n") + }, + opts: func(ios *iostreams.IOStreams, dir string, reg *httpmock.Registry) *PublishOptions { + t.Helper() return &PublishOptions{ IO: ios, Dir: dir, Tag: "v1.0.0", // same as stubAllSecureRemote's existing tag - GitClient: &git.Client{}, + GitClient: newTestGitClient(), HttpClient: func() (*http.Client, error) { return &http.Client{Transport: reg}, nil }, host: "github.com", } @@ -1117,15 +1165,17 @@ 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 { - t.Helper() - initGitRepo(t, dir, map[string]string{ + cmdStubs: func(cs *run.CommandStubber) { + stubGitRemote(cs, map[string]string{ "origin": "https://github.com/monalisa/skills-repo.git", }) + }, + opts: func(ios *iostreams.IOStreams, dir string, reg *httpmock.Registry) *PublishOptions { + t.Helper() return &PublishOptions{ IO: ios, Dir: dir, - GitClient: &git.Client{}, + GitClient: newTestGitClient(), HttpClient: func() (*http.Client, error) { return &http.Client{Transport: reg}, nil }, host: "github.com", } @@ -1215,12 +1265,15 @@ func TestPublishRun(t *testing.T) { }), ) }, + cmdStubs: func(cs *run.CommandStubber) { + stubGitRemote(cs, map[string]string{ + "origin": "https://github.com/monalisa/skills-repo.git", + }) + stubEnsurePushed(cs, "main") + }, opts: func(ios *iostreams.IOStreams, dir string, reg *httpmock.Registry) *PublishOptions { t.Helper() confirmCall := 0 - initGitRepo(t, dir, map[string]string{ - "origin": "https://github.com/monalisa/skills-repo.git", - }) return &PublishOptions{ IO: ios, Dir: dir, @@ -1236,7 +1289,7 @@ func TestPublishRun(t *testing.T) { return "v1.0.0", nil // accept suggested tag }, }, - GitClient: &git.Client{}, + GitClient: newTestGitClient(), HttpClient: func() (*http.Client, error) { return &http.Client{Transport: reg}, nil }, host: "github.com", } @@ -1274,11 +1327,14 @@ func TestPublishRun(t *testing.T) { }), ) }, - opts: func(ios *iostreams.IOStreams, dir string, reg *httpmock.Registry) *PublishOptions { - t.Helper() - initGitRepo(t, dir, map[string]string{ + cmdStubs: func(cs *run.CommandStubber) { + stubGitRemote(cs, map[string]string{ "origin": "https://github.com/monalisa/skills-repo.git", }) + stubEnsurePushed(cs, "main") + }, + opts: func(ios *iostreams.IOStreams, dir string, reg *httpmock.Registry) *PublishOptions { + t.Helper() return &PublishOptions{ IO: ios, Dir: dir, @@ -1293,7 +1349,7 @@ func TestPublishRun(t *testing.T) { return "beta-1", nil }, }, - GitClient: &git.Client{}, + GitClient: newTestGitClient(), HttpClient: func() (*http.Client, error) { return &http.Client{Transport: reg}, nil }, host: "github.com", } @@ -1325,12 +1381,15 @@ func TestPublishRun(t *testing.T) { httpmock.JSONResponse(map[string]interface{}{"default_branch": "main"}), ) }, + cmdStubs: func(cs *run.CommandStubber) { + stubGitRemote(cs, map[string]string{ + "origin": "https://github.com/monalisa/skills-repo.git", + }) + stubEnsurePushed(cs, "main") + }, opts: func(ios *iostreams.IOStreams, dir string, reg *httpmock.Registry) *PublishOptions { t.Helper() confirmCall := 0 - initGitRepo(t, dir, map[string]string{ - "origin": "https://github.com/monalisa/skills-repo.git", - }) return &PublishOptions{ IO: ios, Dir: dir, @@ -1349,7 +1408,7 @@ func TestPublishRun(t *testing.T) { return "v1.0.1", nil }, }, - GitClient: &git.Client{}, + GitClient: newTestGitClient(), HttpClient: func() (*http.Client, error) { return &http.Client{Transport: reg}, nil }, host: "github.com", } @@ -1394,11 +1453,14 @@ func TestPublishRun(t *testing.T) { }), ) }, - opts: func(ios *iostreams.IOStreams, dir string, reg *httpmock.Registry) *PublishOptions { - t.Helper() - initGitRepo(t, dir, map[string]string{ + cmdStubs: func(cs *run.CommandStubber) { + stubGitRemote(cs, map[string]string{ "origin": "https://github.com/monalisa/skills-repo.git", }) + stubEnsurePushed(cs, "main") + }, + opts: func(ios *iostreams.IOStreams, dir string, reg *httpmock.Registry) *PublishOptions { + t.Helper() return &PublishOptions{ IO: ios, Dir: dir, @@ -1413,7 +1475,7 @@ func TestPublishRun(t *testing.T) { return "v1.0.1", nil }, }, - GitClient: &git.Client{}, + GitClient: newTestGitClient(), HttpClient: func() (*http.Client, error) { return &http.Client{Transport: reg}, nil }, host: "github.com", } @@ -1438,6 +1500,12 @@ func TestPublishRun(t *testing.T) { tt.stubs(reg) } + if tt.cmdStubs != nil { + cs, cmdTeardown := run.Stub() + defer cmdTeardown(t) + tt.cmdStubs(cs) + } + opts := tt.opts(ios, dir, reg) err := publishRun(opts) @@ -1461,22 +1529,17 @@ func TestPublishRun(t *testing.T) { } func TestDetectGitHubRemote_UsesDir(t *testing.T) { - // Create two separate git repos: "cwd-repo" simulates the working directory - // and "target-repo" simulates the directory argument passed to publish. - cwdRepo := t.TempDir() - initGitRepo(t, cwdRepo, map[string]string{ - "origin": "https://github.com/monalisa/cwd-repo.git", + cs, cmdTeardown := run.Stub() + defer cmdTeardown(t) + stubGitRemote(cs, map[string]string{ + "origin": "https://github.com/monalisa/target-repo.git", }) + cwdRepo := t.TempDir() targetRepo := t.TempDir() - initGitRepo(t, targetRepo, map[string]string{ - "origin": "https://github.com/monalisa/target-repo.git", - }) - // gitClient points at cwd-repo (simulating factory-provided client) - gitClient := &git.Client{RepoDir: cwdRepo} + gitClient := &git.Client{GitPath: "some/path/git", RepoDir: cwdRepo} - // detectGitHubRemote should use targetRepo's remotes, not cwdRepo's repo, err := detectGitHubRemote(gitClient, targetRepo) require.NoError(t, err) require.NotNil(t, repo) @@ -1485,24 +1548,14 @@ func TestDetectGitHubRemote_UsesDir(t *testing.T) { } func TestPublishRun_DirArgUsesTargetRemote(t *testing.T) { - // Regression test: when a directory argument is provided, remote detection - // must use that directory's git remotes, not the factory client's directory. - // - // Scenario: - // 1. User is in cwd-repo (has remote → monalisa/cwd-repo) - // 2. User runs: gh skill publish /path/to/target-repo - // 3. target-repo has remote → monalisa/target-repo - // 4. API calls must go to target-repo, NOT cwd-repo - - cwdRepo := t.TempDir() - initGitRepo(t, cwdRepo, map[string]string{ - "origin": "https://github.com/monalisa/cwd-repo.git", + cs, cmdTeardown := run.Stub() + defer cmdTeardown(t) + stubGitRemote(cs, map[string]string{ + "origin": "https://github.com/monalisa/target-repo.git", }) + cwdRepo := t.TempDir() targetRepo := t.TempDir() - initGitRepo(t, targetRepo, map[string]string{ - "origin": "https://github.com/monalisa/target-repo.git", - }) writeSkill(t, targetRepo, "my-skill", heredoc.Doc(` --- @@ -1520,17 +1573,13 @@ func TestPublishRun_DirArgUsesTargetRemote(t *testing.T) { reg := &httpmock.Registry{} defer reg.Verify(t) - - // Stub API calls for target-repo (the correct repo). - // If the bug is present, these stubs won't be called because the code - // would try to hit cwd-repo endpoints instead, and reg.Verify would fail. stubAllSecureRemote(reg, "monalisa", "target-repo") err := publishRun(&PublishOptions{ IO: ios, Dir: targetRepo, DryRun: true, - GitClient: &git.Client{RepoDir: cwdRepo}, + GitClient: &git.Client{GitPath: "some/path/git", RepoDir: cwdRepo}, HttpClient: func() (*http.Client, error) { return &http.Client{Transport: reg}, nil }, host: "github.com", }) @@ -1547,112 +1596,36 @@ func writeSkill(t *testing.T, dir, name, content string) { require.NoError(t, os.WriteFile(filepath.Join(skillDir, "SKILL.md"), []byte(content), 0o644)) } -// runGitInDir runs a git command in the given directory with isolation env vars. -func runGitInDir(t *testing.T, dir string, args ...string) { - t.Helper() - cmd := exec.Command("git", append([]string{"-C", dir}, args...)...) - cmd.Env = append(os.Environ(), "GIT_CONFIG_NOSYSTEM=1", "HOME="+dir) - out, err := cmd.CombinedOutput() - require.NoError(t, err, "git %v: %s", args, out) -} - -// newTestGitClientWithUpstream creates a git repo with a local bare "remote" -// and an initial commit, so we can test push/rev-list behavior realistically. -// It returns the git client and the working directory path. -func newTestGitClientWithUpstream(t *testing.T) (*git.Client, string) { - t.Helper() - parentDir := t.TempDir() - bareDir := filepath.Join(parentDir, "upstream.git") - workDir := filepath.Join(parentDir, "work") - - gitEnv := append(os.Environ(), "GIT_CONFIG_NOSYSTEM=1", "HOME="+parentDir) - - run := func(dir string, args ...string) { - t.Helper() - c := exec.Command("git", append([]string{"-C", dir}, args...)...) - c.Env = gitEnv - out, err := c.CombinedOutput() - require.NoError(t, err, "git %v: %s", args, out) - } - - // Create bare upstream - require.NoError(t, os.MkdirAll(bareDir, 0o755)) - run(bareDir, "init", "--bare", "--initial-branch=main") - - // Clone into working dir - c := exec.Command("git", "clone", bareDir, workDir) - c.Env = gitEnv - out, err := c.CombinedOutput() - require.NoError(t, err, "git clone: %s", out) - - run(workDir, "config", "user.email", "monalisa@github.com") - run(workDir, "config", "user.name", "Monalisa Octocat") - - // Create initial commit and push - require.NoError(t, os.WriteFile(filepath.Join(workDir, "README.md"), []byte("# Test"), 0o644)) - run(workDir, "add", ".") - run(workDir, "commit", "-m", "initial commit") - run(workDir, "push", "origin", "main") - - return &git.Client{ - RepoDir: workDir, - GitPath: "git", - Stderr: &bytes.Buffer{}, - Stdin: &bytes.Buffer{}, - Stdout: &bytes.Buffer{}, - }, workDir -} - func TestEnsurePushed(t *testing.T) { tests := []struct { name string - setup func(t *testing.T, workDir string) - verify func(t *testing.T, workDir string) + cmdStubs func(*run.CommandStubber) wantErr string wantStderr string }{ { name: "no unpushed commits is a no-op", - setup: func(_ *testing.T, _ string) { - // initial commit already pushed by helper + cmdStubs: func(cs *run.CommandStubber) { + cs.Register(`git( .+)? symbolic-ref --quiet HEAD`, 0, "refs/heads/main\n") + cs.Register(`git( .+)? rev-list --count @\{push\}\.\.HEAD`, 0, "0\n") }, }, { name: "unpushed commits are pushed automatically", - setup: func(t *testing.T, workDir string) { - t.Helper() - require.NoError(t, os.WriteFile(filepath.Join(workDir, "new.txt"), []byte("new"), 0o644)) - runGitInDir(t, workDir, "add", ".") - runGitInDir(t, workDir, "commit", "-m", "unpushed change") - }, - verify: func(t *testing.T, workDir string) { - t.Helper() - // After push, rev-list should show 0 unpushed commits - cmd := exec.Command("git", "-C", workDir, "rev-list", "--count", "@{push}..HEAD") - cmd.Env = append(os.Environ(), "GIT_CONFIG_NOSYSTEM=1", "HOME="+workDir) - out, err := cmd.CombinedOutput() - require.NoError(t, err, "rev-list: %s", out) - assert.Equal(t, "0", strings.TrimSpace(string(out))) + cmdStubs: func(cs *run.CommandStubber) { + cs.Register(`git( .+)? symbolic-ref --quiet HEAD`, 0, "refs/heads/main\n") + cs.Register(`git( .+)? rev-list --count @\{push\}\.\.HEAD`, 0, "1\n") + cs.Register(`git( .+)? push --set-upstream origin HEAD:refs/heads/main`, 0, "") }, wantStderr: "Pushing main to origin", }, { - name: "new branch never pushed is pushed automatically", - setup: func(t *testing.T, workDir string) { - t.Helper() - runGitInDir(t, workDir, "checkout", "-b", "feature") - require.NoError(t, os.WriteFile(filepath.Join(workDir, "feat.txt"), []byte("feat"), 0o644)) - runGitInDir(t, workDir, "add", ".") - runGitInDir(t, workDir, "commit", "-m", "new branch commit") - }, - verify: func(t *testing.T, workDir string) { - t.Helper() - // After push, the branch should exist on the remote - cmd := exec.Command("git", "-C", workDir, "rev-list", "--count", "@{push}..HEAD") - cmd.Env = append(os.Environ(), "GIT_CONFIG_NOSYSTEM=1", "HOME="+workDir) - out, err := cmd.CombinedOutput() - require.NoError(t, err, "rev-list: %s", out) - assert.Equal(t, "0", strings.TrimSpace(string(out))) + name: "new branch is not pushed is pushed automatically", + cmdStubs: func(cs *run.CommandStubber) { + cs.Register(`git( .+)? symbolic-ref --quiet HEAD`, 0, "refs/heads/feature\n") + // rev-list fails when branch is not pushed + cs.Register(`git( .+)? rev-list --count @\{push\}\.\.HEAD`, 1, "") + cs.Register(`git( .+)? push --set-upstream origin HEAD:refs/heads/feature`, 0, "") }, wantStderr: "Pushing feature to origin", }, @@ -1660,8 +1633,11 @@ func TestEnsurePushed(t *testing.T) { for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { - gitClient, workDir := newTestGitClientWithUpstream(t) - tt.setup(t, workDir) + cs, cmdTeardown := run.Stub() + defer cmdTeardown(t) + tt.cmdStubs(cs) + + workDir := t.TempDir() ios, _, _, stderr := iostreams.Test() ios.SetStdoutTTY(true) @@ -1669,7 +1645,7 @@ func TestEnsurePushed(t *testing.T) { opts := &PublishOptions{ IO: ios, - GitClient: gitClient, + GitClient: &git.Client{GitPath: "some/path/git", RepoDir: workDir}, } err := ensurePushed(opts, workDir, "origin") @@ -1683,9 +1659,6 @@ func TestEnsurePushed(t *testing.T) { if tt.wantStderr != "" { assert.Contains(t, stderr.String(), tt.wantStderr) } - if tt.verify != nil { - tt.verify(t, workDir) - } }) } } From c5c7d790c5e19b2d68997d2799f15c7684687ab0 Mon Sep 17 00:00:00 2001 From: Sam Morrow Date: Thu, 16 Apr 2026 19:21:35 +0200 Subject: [PATCH 12/12] Update publish_test.go Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com> --- pkg/cmd/skills/publish/publish_test.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/pkg/cmd/skills/publish/publish_test.go b/pkg/cmd/skills/publish/publish_test.go index 29142d44f47..f83117b5b5e 100644 --- a/pkg/cmd/skills/publish/publish_test.go +++ b/pkg/cmd/skills/publish/publish_test.go @@ -1620,7 +1620,7 @@ func TestEnsurePushed(t *testing.T) { wantStderr: "Pushing main to origin", }, { - name: "new branch is not pushed is pushed automatically", + name: "new branch that has not been pushed is pushed automatically", cmdStubs: func(cs *run.CommandStubber) { cs.Register(`git( .+)? symbolic-ref --quiet HEAD`, 0, "refs/heads/feature\n") // rev-list fails when branch is not pushed