Address post-merge review feedback for skills commands#13185
Merged
SamMorrowDrums merged 3 commits intotrunkfrom Apr 16, 2026
Merged
Address post-merge review feedback for skills commands#13185SamMorrowDrums merged 3 commits intotrunkfrom
SamMorrowDrums merged 3 commits intotrunkfrom
Conversation
- 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>
Contributor
There was a problem hiding this comment.
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 skillas 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>
babakks
approved these changes
Apr 16, 2026
| IO: ios, | ||
| Dir: dir, | ||
| GitClient: &git.Client{}, | ||
| HttpClient: func() (*http.Client, error) { return &http.Client{}, nil }, |
Member
There was a problem hiding this comment.
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).
Comment on lines
+773
to
+780
| // 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 | ||
| } |
Member
There was a problem hiding this comment.
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 file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Addresses review feedback from PRs #13167, #13168, #13170, and #13171 that was deferred to post-merge follow-ups on the
sm/add-skills-commandbranch.Closes #13184
Changes
PR #13168 — Use factory pattern for HTTP client injection
opts.clientfield fromPublishOptionsopts.HttpClientto return a stubbed*http.Clientviahttpmock.Registry, matching the standard factory pattern used across the codebasepublishRunPR #13170 — Search deduplication improvements
testName→nameinTestMatchSkillPathtest struct (discovery_test.go)deduplicateResultswith a typedskillResultKeystruct — more idiomatic Go, avoids potential key collisiondeduplicateResultsnow normalizes repo, namespace, and skill name withstrings.ToLower(), matching the behavior ofdeduplicateByNamePR #13171 — Remote selection and push UX
detectGitHubRemotenow iteratesRemotes()directly (which returns upstream > github > origin > rest) instead of manually trying origin first then falling backSuccessIconbefore push; now shows a plain "Pushing..." message, thenSuccessIcononly after push succeedsDeferred
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