Address post-merge review feedback for skills commands by SamMorrowDrums · Pull Request #13185 · cli/cli

SamMorrowDrums · 2026-04-16T14:20:26Z

Summary

Addresses review feedback from PRs #13167, #13168, #13170, and #13171 that was deferred to post-merge follow-ups on the sm/add-skills-command branch.

Closes #13184

Changes

PR #13168 — Use factory pattern for HTTP client injection

Removed direct opts.client field from PublishOptions
Tests now set opts.HttpClient to return a stubbed *http.Client via httpmock.Registry, matching the standard factory pattern used across the codebase
Removed "test-aware" nil-client fallback from publishRun

PR #13170 — Search deduplication improvements

Renamed testName → name in TestMatchSkillPath test struct (discovery_test.go)
Typed struct keys for dedup map: Replaced string-concatenated map key in deduplicateResults with a typed skillResultKey struct — more idiomatic Go, avoids potential key collision
Case-insensitive deduplication: deduplicateResults now normalizes repo, namespace, and skill name with strings.ToLower(), matching the behavior of deduplicateByName
Whitespace fix: Already resolved in a prior commit (verified all tabs)

PR #13171 — Remote selection and push UX

Simplified remote selection: detectGitHubRemote now iterates Remotes() directly (which returns upstream > github > origin > rest) instead of manually trying origin first then falling back
Fixed push icon timing: Removed premature SuccessIcon before push; now shows a plain "Pushing..." message, then SuccessIcon only after push succeeds

Deferred

Replace real git in tests with run.CommandStubber (PR feat(skills): auto-push unpushed commits before publish #13171 feedback): Converting 20+ test cases from real git to command stubs is a substantial refactor best handled in a separate issue

- 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>

Copilot

Pull request overview

This PR continues the “skills” CLI work by wiring the top-level gh skill command and updating several skills subcommands and internals (search/preview/update), including new supporting packages (lockfile + cross-platform file locking) and small git client helpers.

Changes:

Add/enable gh skill as a top-level command and introduce/extend skills subcommands (search, preview, update).
Improve skills search result handling (namespace-aware + case-insensitive dedup, typed dedup keys) and update related tests/acceptance coverage.
Add skills installation bookkeeping via a lockfile with cross-platform file locking; extend git client utilities used by the skills flows.

Show a summary per file

File	Description
pkg/cmd/skills/skills.go	Adds the top-level `skill` command and wires subcommands including `update`.
pkg/cmd/root/root.go	Registers the new `skill` command on the root CLI.
pkg/cmd/skills/search/search.go	Improves search deduplication + namespacing, and install UX from interactive search.
pkg/cmd/skills/search/search_test.go	Expands/updates unit coverage for search behavior changes.
pkg/cmd/skills/preview/preview.go	Enhances preview flow (file tree + browsing additional files).
pkg/cmd/skills/preview/preview_test.go	Adds/updates preview test coverage for new behavior and limits.
pkg/cmd/skills/update/update.go	Implements `gh skill update` for scanning/updating installed skills.
internal/skills/discovery/discovery.go	Extends discovery helpers and adds namespace-aware path parsing.
internal/skills/frontmatter/frontmatter.go	Adds metadata injection helpers for GitHub/local installs.
internal/skills/installer/installer.go	Adds installer behavior including concurrency + lockfile recording.
internal/skills/lockfile/lockfile.go	Introduces `.skill-lock.json` recording with serialized access via file locks.
internal/flock/*	Adds cross-platform non-blocking file locking implementation + tests.
internal/skills/source/source.go	Adds parsing/validation helpers for GitHub repo metadata.
internal/skills/registry/registry.go	Adds agent host registry + install dir resolution utilities.
git/client.go	Adds `RemoteURL`, `IsIgnored`, and `ShortSHA` helpers used by skills flows.
go.mod	Promotes `golang.org/x/sys` to a direct dependency (for Windows locking).
acceptance/testdata/skills/*	Adds/updates acceptance coverage for skills commands (install/search/preview/publish/update).
acceptance/acceptance_test.go	Adds a `TestSkills` acceptance runner.
.gitignore	Ignores a `gh` binary/artifact.

Copilot's findings

Tip

Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Files reviewed: 54/55 changed files
Comments generated: 1

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

BagToad

LGTM

babakks

LGTM! Thanks! 🍻

babakks · 2026-04-16T15:26:06Z

+		IO:         ios,
+		Dir:        dir,
+		GitClient:  &git.Client{},
+		HttpClient: func() (*http.Client, error) { return &http.Client{}, nil },


It's a nitpick since this scenario does not really need an HTTP client.

nitpick: let's not pass a real HTTP client. I think returning a nil should also be fine in this case (as there's no API call going to happen).

babakks · 2026-04-16T15:28:02Z

+// 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
+}


nitpick: this doesn't need to be a top-level type as it's just used within deduplicateResults.

- 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>

This MR contains the following updates: | Package | Update | Change | |---|---|---| | [cli/cli](https://github.com/cli/cli) | minor | `v2.89.0` → `v2.90.0` | MR created with the help of [el-capitano/tools/renovate-bot](https://gitlab.com/el-capitano/tools/renovate-bot). **Proposed changes to behavior should be submitted there as MRs.** --- ### Release Notes <details> <summary>cli/cli (cli/cli)</summary> ### [`v2.90.0`](https://github.com/cli/cli/releases/tag/v2.90.0): GitHub CLI 2.90.0 [Compare Source](cli/cli@v2.89.0...v2.90.0) #### Manage agent skills with `gh skill` (Public Preview) [Agent skills](https://agentskills.io) are portable sets of instructions, scripts, and resources that teach AI coding agents how to perform specific tasks. The new `gh skill` command makes it easy to discover, install, manage, and publish agent skills from GitHub repositories - right from the CLI. ``` # Discover skills gh skill search copilot # Preview a skill without installing gh skill preview github/awesome-copilot documentation-writer # Install a skill gh skill install github/awesome-copilot documentation-writer # Pin to a specific version gh skill install github/awesome-copilot documentation-writer --pin v1.2.0 # Check installed skills for updates gh skill update --all # Validate and publish your own skills gh skill publish --dry-run ``` Skills are automatically installed to the correct directory for your agent host. `gh skill` supports GitHub Copilot, Claude Code, Cursor, Codex, Gemini CLI, and Antigravity. Target a specific agent and scope with `--agent` and `--scope` flags. `gh skill publish` validates skills against the [Agent Skills specification](https://agentskills.io/specification) and checks remote settings like tag protection and immutable releases to improve supply chain security. Read the full announcement on the [GitHub Blog](https://github.blog/changelog/2026-04-16-manage-agent-skills-with-github-cli/). `gh skill` is launching in public preview and is subject to change without notice. #### Official extension suggestions When you run a command that matches a known official extension that isn't installed (e.g. `gh stack`), the CLI now offers to install it instead of showing a generic "unknown command" error. This feature is available for [github/gh-aw](https://github.com/github/gh-aw) and [github/gh-stack](https://github.com/github/gh-stack). When possible, you'll be prompted to install immediately. When prompting isn't possible, the CLI prints the `gh extension install` command to run. #### `gh extension install` no longer requires authentication `gh extension install` previously required a valid auth token even though it only needs to download a public release asset. The auth check has been removed, so you can install extensions without being logged in. #### What's Changed ##### ✨ Features - Add `gh skill` command group: install, preview, search, update, publish by [@SamMorrowDrums](https://github.com/SamMorrowDrums) in [#13165](cli/cli#13165) - Suggest and install official extensions for unknown commands by [@BagToad](https://github.com/BagToad) in [#13175](cli/cli#13175) - `gh skill publish`: auto-push unpushed commits before publish by [@SamMorrowDrums](https://github.com/SamMorrowDrums) in [#13171](cli/cli#13171) - Disable auth check for `gh extension install` by [@BagToad](https://github.com/BagToad) in [#13176](cli/cli#13176) ##### 🐛 Fixes - Fix infinite loop in `gh release list --limit 0` by [@Bahtya](https://github.com/Bahtya) in [#13097](cli/cli#13097) - Ensure `api` and `auth` commands record agentic invocations by [@williammartin](https://github.com/williammartin) in [#13046](cli/cli#13046) - Disable auth check for local-only skill flags by [@SamMorrowDrums](https://github.com/SamMorrowDrums) in [#13173](cli/cli#13173) - URL-encode parentPath in skills discovery API call by [@SamMorrowDrums](https://github.com/SamMorrowDrums) in [#13172](cli/cli#13172) - Fix: use target directory remotes in skills publish by [@SamMorrowDrums](https://github.com/SamMorrowDrums) in [#13169](cli/cli#13169) - Fix: preserve namespace in skills search deduplication by [@SamMorrowDrums](https://github.com/SamMorrowDrums) in [#13170](cli/cli#13170) ##### 📚 Docs & Chores - docs: include PGP key fingerprints by [@babakks](https://github.com/babakks) in [#13112](cli/cli#13112) - docs: add sha/md5 checksums of keyring files by [@babakks](https://github.com/babakks) in [#13150](cli/cli#13150) - docs: fix SHA512 checksum for GPG key by [@timsu92](https://github.com/timsu92) in [#13157](cli/cli#13157) - docs(skill): polish skill commandset docs by [@babakks](https://github.com/babakks) in [#13183](cli/cli#13183) - Document dependency CVE policy in SECURITY.md by [@BagToad](https://github.com/BagToad) in [#13119](cli/cli#13119) - Replace github.com/golang/snappy with klauspost/compress/snappy by [@thaJeztah](https://github.com/thaJeztah) in [#13048](cli/cli#13048) - chore: bump to go1.26.2 by [@babakks](https://github.com/babakks) in [#13116](cli/cli#13116) - chore: delete experimental script/debian-devel by [@babakks](https://github.com/babakks) in [#13127](cli/cli#13127) - Suggest first party extensions by [@williammartin](https://github.com/williammartin) in [#13182](cli/cli#13182) - Add cli/skill-reviewers as CODEOWNERS for skills packages by [@BagToad](https://github.com/BagToad) in [#13189](cli/cli#13189) - Add [@cli/code-reviewers](https://github.com/cli/code-reviewers) to all CODEOWNERS rules by [@BagToad](https://github.com/BagToad) in [#13190](cli/cli#13190) - Address post-merge review feedback for skills commands by [@SamMorrowDrums](https://github.com/SamMorrowDrums) in [#13185](cli/cli#13185) - Fix skills-publish-dry-run acceptance test error message mismatch by [@SamMorrowDrums](https://github.com/SamMorrowDrums) in [#13187](cli/cli#13187) - Skills: replace real git in publish tests with CommandStubber by [@SamMorrowDrums](https://github.com/SamMorrowDrums) in [#13188](cli/cli#13188) - Remove redundant nil-client fallback in skills publish by [@SamMorrowDrums](https://github.com/SamMorrowDrums) in [#13168](cli/cli#13168) - Publish: use shared discovery logic instead of requiring skills/ directory by [@SamMorrowDrums](https://github.com/SamMorrowDrums) in [#13167](cli/cli#13167) #####

Dependencies - chore(deps): bump github.com/klauspost/compress from 1.18.4 to 1.18.5 by [@dependabot](https://github.com/dependabot)\[bot] in [#13071](cli/cli#13071) - chore(deps): bump github.com/yuin/goldmark from 1.7.16 to 1.8.2 by [@dependabot](https://github.com/dependabot)\[bot] in [#13045](cli/cli#13045) - chore(deps): bump charm.land/bubbles/v2 from 2.0.0 to 2.1.0 by [@dependabot](https://github.com/dependabot)\[bot] in [#13051](cli/cli#13051) - chore(deps): bump github.com/sigstore/timestamp-authority/v2 from 2.0.3 to 2.0.6 by [@dependabot](https://github.com/dependabot)\[bot] in [#13152](cli/cli#13152) - chore(deps): bump github.com/google/go-containerregistry from 0.21.3 to 0.21.4 by [@dependabot](https://github.com/dependabot)\[bot] in [#13129](cli/cli#13129) - chore(deps): bump github.com/sigstore/protobuf-specs from 0.5.0 to 0.5.1 by [@dependabot](https://github.com/dependabot)\[bot] in [#13128](cli/cli#13128) - chore(deps): bump github.com/in-toto/attestation from 1.1.2 to 1.2.0 by [@dependabot](https://github.com/dependabot)\[bot] in [#13044](cli/cli#13044) - chore(deps): bump advanced-security/filter-sarif from 1.0.1 to 1.1 by [@dependabot](https://github.com/dependabot)\[bot] in [#12918](cli/cli#12918) - chore(deps): bump google.golang.org/grpc from 1.79.3 to 1.80.0 by [@dependabot](https://github.com/dependabot)\[bot] in [#13076](cli/cli#13076) - chore(deps): bump github.com/hashicorp/go-version from 1.8.0 to 1.9.0 by [@dependabot](https://github.com/dependabot)\[bot] in [#13065](cli/cli#13065) #### New Contributors - [@thaJeztah](https://github.com/thaJeztah) made their first contribution in [#13048](cli/cli#13048) - [@Bahtya](https://github.com/Bahtya) made their first contribution in [#13097](cli/cli#13097) - [@timsu92](https://github.com/timsu92) made their first contribution in [#13157](cli/cli#13157) - [@SamMorrowDrums](https://github.com/SamMorrowDrums) made their first contribution in [#13173](cli/cli#13173) **Full Changelog**: <cli/cli@v2.89.0...v2.90.0> </details> --- ### Configuration 📅 **Schedule**: (UTC) - Branch creation - At any time (no schedule defined) - Automerge - At any time (no schedule defined) 🚦 **Automerge**: Disabled by config. Please merge this manually once you are satisfied. ♻ **Rebasing**: Whenever MR becomes conflicted, or you tick the rebase/retry checkbox. 🔕 **Ignore**: Close this MR and you won't be reminded about this update again. --- - [ ] If you want to rebase/retry this MR, check this box --- This MR has been generated by [Renovate Bot](https://github.com/renovatebot/renovate).

SamMorrowDrums requested a review from a team as a code owner April 16, 2026 14:20

SamMorrowDrums requested review from Copilot and williammartin April 16, 2026 14:20

Copilot started reviewing on behalf of SamMorrowDrums April 16, 2026 14:21 View session

SamMorrowDrums mentioned this pull request Apr 16, 2026

Skills: replace real git in publish tests with CommandStubber #13186

Closed

Copilot AI reviewed Apr 16, 2026

View reviewed changes

Comment thread internal/skills/installer/installer.go

SamMorrowDrums mentioned this pull request Apr 16, 2026

Skills: replace real git in publish tests with CommandStubber #13188

Merged

SamMorrowDrums changed the base branch from trunk to sm/add-skills-command April 16, 2026 15:02

style: fix gofmt alignment in publish tests

9d00ecc

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

Base automatically changed from sm/add-skills-command to trunk April 16, 2026 15:05

BagToad approved these changes Apr 16, 2026

View reviewed changes

babakks approved these changes Apr 16, 2026

View reviewed changes

fix: apply review feedback — nil HttpClient, local dedup type

de204f8

- 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>

SamMorrowDrums requested a review from a team April 16, 2026 16:50

SamMorrowDrums enabled auto-merge April 16, 2026 16:57

SamMorrowDrums merged commit d7961f1 into trunk Apr 16, 2026
11 checks passed

SamMorrowDrums deleted the sammorrowdrums/skills-post-merge-review-followups branch April 16, 2026 16:59

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Address post-merge review feedback for skills commands#13185

Address post-merge review feedback for skills commands#13185
SamMorrowDrums merged 3 commits into
trunkfrom
sammorrowdrums/skills-post-merge-review-followups

SamMorrowDrums commented Apr 16, 2026

Uh oh!

Copilot AI left a comment

Uh oh!

Uh oh!

BagToad left a comment

Uh oh!

babakks left a comment

Uh oh!

babakks Apr 16, 2026

Uh oh!

babakks Apr 16, 2026

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

Conversation

SamMorrowDrums commented Apr 16, 2026

Summary

Changes

PR #13168 — Use factory pattern for HTTP client injection

PR #13170 — Search deduplication improvements

PR #13171 — Remote selection and push UX

Deferred

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Copilot's findings

Uh oh!

Uh oh!

BagToad left a comment

Choose a reason for hiding this comment

Uh oh!

babakks left a comment

Choose a reason for hiding this comment

Uh oh!

babakks Apr 16, 2026

Choose a reason for hiding this comment

Uh oh!

babakks Apr 16, 2026

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants