Remove redundant nil-client fallback in skills publish#13168
Merged
SamMorrowDrums merged 1 commit intosm/add-skills-commandfrom Apr 16, 2026
Merged
Conversation
Contributor
There was a problem hiding this comment.
Pull request overview
Introduces the new preview “skills” feature set in the GitHub CLI by adding a top-level gh skill / gh skills command group with supporting subcommands and underlying skills infrastructure (discovery, install metadata, lockfile, and cross-platform file locking), plus wiring into root and acceptance tests.
Changes:
- Add
gh skillcommand group and newsearch,preview, andupdatecommands (plus tests). - Add internal skills infrastructure (discovery, registry, metadata parsing/injection, lockfile with cross-platform flocking).
- Wire commands into the root command and extend acceptance coverage; add small git client helpers.
Show a summary per file
| File | Description |
|---|---|
| pkg/cmd/skills/update/update.go | Implements gh skill update by scanning installed skills, checking remote SHAs, and reinstalling updates. |
| pkg/cmd/skills/skills.go | Adds the gh skill/gh skills command group and attaches subcommands. |
| pkg/cmd/skills/search/search.go | Implements gh skill search using GitHub Code Search with relevance ranking and optional interactive install. |
| pkg/cmd/skills/search/search_test.go | Unit tests for search behavior, pagination, relevance ranking, and rate-limit messaging. |
| pkg/cmd/skills/preview/preview.go | Implements gh skill preview including tree display, pager rendering, and interactive file browsing. |
| pkg/cmd/skills/preview/preview_test.go | Unit tests for preview flows, interactive selection, and render limits. |
| pkg/cmd/root/root.go | Registers the new skills command group on the root command. |
| internal/skills/source/source.go | Adds helpers to build/parse repo metadata and enforce supported host restrictions. |
| internal/skills/source/source_test.go | Tests repo URL building/parsing and supported-host validation. |
| internal/skills/registry/registry.go | Defines supported agent hosts and resolves install locations by scope. |
| internal/skills/registry/registry_test.go | Tests agent lookup, install dir resolution, scope labels, and remote URL parsing. |
| internal/skills/lockfile/lockfile.go | Adds .skill-lock.json management with file locking to avoid concurrent write races. |
| internal/skills/lockfile/lockfile_test.go | Tests lockfile creation, updates, recovery from corruption/version mismatch, and lock contention. |
| internal/skills/installer/installer.go | Implements remote/local install logic with metadata injection and path traversal protections. |
| internal/skills/installer/installer_test.go | Tests local/remote install behaviors, symlink handling, traversal blocking, lockfile writing, and progress callbacks. |
| internal/skills/frontmatter/frontmatter.go | Parses/serializes frontmatter and injects GitHub/local metadata into SKILL.md. |
| internal/skills/frontmatter/frontmatter_test.go | Tests parsing and metadata injection/serialization behaviors. |
| internal/skills/discovery/discovery.go | Implements skill discovery (remote + local), ref resolution, file enumeration, and blob fetching. |
| internal/skills/discovery/collisions.go | Adds detection/formatting of install-name collisions. |
| internal/skills/discovery/collisions_test.go | Tests collision detection and formatting. |
| internal/flock/flock.go | Adds OS-agnostic sentinel error for lock contention. |
| internal/flock/flock_unix.go | Unix implementation of non-blocking exclusive file locks. |
| internal/flock/flock_windows.go | Windows implementation of non-blocking exclusive file locks using Win32 APIs. |
| internal/flock/flock_test.go | Tests lock acquisition, contention, and basic file usability while locked. |
| go.mod | Promotes golang.org/x/sys to a direct dependency (needed for Windows locking). |
| git/client.go | Adds RemoteURL, IsIgnored, and ShortSHA helpers on the git client. |
| git/client_test.go | Tests for the new git client helper methods. |
| acceptance/acceptance_test.go | Adds a skills acceptance test suite runner. |
| acceptance/testdata/skills/skills-update.txtar | Acceptance coverage for update flow in custom directory mode. |
| acceptance/testdata/skills/skills-update-noinstalled.txtar | Acceptance coverage for update behavior when no installed skills exist. |
| acceptance/testdata/skills/skills-search.txtar | Acceptance coverage for basic search and JSON output flags. |
| acceptance/testdata/skills/skills-search-page.txtar | Acceptance coverage for search pagination. |
| acceptance/testdata/skills/skills-search-noresults.txtar | Acceptance coverage for no-results behavior in non-TTY mode. |
| acceptance/testdata/skills/skills-preview.txtar | Acceptance coverage for preview rendering and not-found errors. |
| acceptance/testdata/skills/skills-preview-noninteractive.txtar | Acceptance coverage for non-interactive preview requiring a skill name. |
| acceptance/testdata/skills/skills-install.txtar | Acceptance coverage for install, metadata injection, lockfile creation, and --dir. |
| acceptance/testdata/skills/skills-install-scope.txtar | Acceptance coverage for project vs user scope install locations. |
| acceptance/testdata/skills/skills-install-pin.txtar | Acceptance coverage for --pin behavior. |
| acceptance/testdata/skills/skills-install-nonexistent-skill.txtar | Acceptance coverage for install failure when skill doesn’t exist. |
| acceptance/testdata/skills/skills-install-nested-files.txtar | Acceptance coverage for installing skills with nested file structures. |
| acceptance/testdata/skills/skills-install-invalid-repo.txtar | Acceptance coverage for install failure when repo doesn’t exist. |
| acceptance/testdata/skills/skills-install-invalid-agent.txtar | Acceptance coverage for invalid --agent validation output. |
| acceptance/testdata/skills/skills-install-from-local.txtar | Acceptance coverage for --from-local install and local metadata injection. |
| acceptance/testdata/skills/skills-install-force.txtar | Acceptance coverage for --force overwrite behavior. |
| acceptance/testdata/skills/skills-publish-lifecycle.txtar | Acceptance coverage for publish/install/preview/update lifecycle using a real repo. |
| acceptance/testdata/skills/skills-publish-dry-run.txtar | Acceptance coverage for publish dry-run validation behavior. |
| .gitignore | Ignores a local gh binary/artifact. |
Copilot's findings
Tip
Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
- Files reviewed: 52/53 changed files
- Comments generated: 2
babakks
approved these changes
Apr 16, 2026
Member
babakks
left a comment
There was a problem hiding this comment.
LGTM, but I recommend you apply the suggested change to keep it consistent with other gh commands.
| // simple errors (missing skills/, bad SKILL.md, etc.) are reported | ||
| // without requiring an HTTP client. | ||
| client := opts.client | ||
| host := opts.host |
Member
There was a problem hiding this comment.
We don't inject HTTP clients for testing in this way. Basically, the test function creates a stubbed HTTP client and make the factory function (opts.HttpClient) return that (example)
That way you never need to involve the test-aware mindset in the command code.
8 tasks
Remove the dead code block that silently swallowed errors from opts.HttpClient() and opts.Config() when creating an API client. Instead, create the client with proper error propagation inside the remote-checks block where it is actually needed. Changes: - Remove the error-swallowing client == nil fallback (lines 363-376) - Create the API client inside the remote repo checks block with proper error returns from HttpClient() and Config() - Resolve host from repoInfo first, then fall back to config - Remove the now-unreachable 'client == nil' early exit before publish Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
74034e2 to
768470d
Compare
SamMorrowDrums
added a commit
that referenced
this pull request
Apr 16, 2026
- 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>
Closed
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.
Stacked on #13165.
Problem
The
publishRunfunction contained a block (lines 363-376) that silently swallowed errors fromopts.HttpClient()andopts.Config()when creating an API client. If either factory function failed, the code would simply skip client creation and continue withclient == nil, eventually producing a vague "could not create API client" message instead of surfacing the actual error.This block was an artifact from migrating away from custom HTTP clients.
Changes
client == nilfallback block — factory errors are now properly returnedrepoInfofirst, then falls back to config (with proper error returns)client == nilearly exit before the publish flow — the client is now guaranteed to exist at that pointTesting
All existing tests pass. No new tests needed — this is a pure refactor that makes errors visible rather than silently swallowing them.