fix: use target directory remotes in skills publish#13169
Merged
SamMorrowDrums merged 2 commits intosm/add-skills-commandfrom Apr 15, 2026
Merged
Conversation
When a directory argument is provided to `gh skill publish`, the remote detection now correctly uses the target directory's git remotes instead of the current working directory's remotes. Previously, `detectGitHubRemote` used the factory-provided git client which pointed to the CWD. This meant that running `gh skill publish /path/to/repo-bar` from inside repo-foo would detect repo-foo's remotes and potentially create the release on the wrong repo. The fix copies the git client and sets `RepoDir` to the target directory, matching the pattern already used by `detectMissingRepoDiagnostic` and `checkInstalledSkillDirs`. Co-authored-by: BagToad <47394200+BagToad@users.noreply.github.com> Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Contributor
There was a problem hiding this comment.
Pull request overview
Fixes incorrect GitHub remote detection in gh skill publish when a target directory is provided, ensuring releases are created against the target repo rather than the current working directory.
Changes:
- Update
detectGitHubRemoteto accept a targetdirand run git remote detection against that directory (via a copied git client withRepoDirset). - Refactor/extend tests to initialize git repos in the skill directory and add regression coverage for the
--dirscenario. - Add an acceptance test covering publishing from one repo while targeting another.
Show a summary per file
| File | Description |
|---|---|
pkg/cmd/skills/publish/publish.go |
Ensures remote detection uses the provided publish directory by copying the git client and setting RepoDir. |
pkg/cmd/skills/publish/publish_test.go |
Updates existing tests to initialize git in the target dir and adds new unit/integration regression tests for dir-based remote detection. |
acceptance/testdata/skills/skills-publish-dir-remote.txtar |
Adds end-to-end acceptance coverage verifying release is created on the target repo when invoked from another repo. |
Copilot's findings
Tip
Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
- Files reviewed: 3/3 changed files
- Comments generated: 1
Comment on lines
+23
to
+28
| func newTestGitClient(t *testing.T, remoteURLs map[string]string) *git.Client { | ||
| t.Helper() | ||
| dir := t.TempDir() | ||
| runGit := func(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) | ||
| } | ||
| runGit("init", "--initial-branch=main") | ||
| runGit("config", "user.email", "monalisa@github.com") | ||
| runGit("config", "user.name", "Monalisa Octocat") | ||
| initGitRepo(t, dir, remoteURLs) | ||
| return &git.Client{RepoDir: dir} | ||
| } |
There was a problem hiding this comment.
newTestGitClient is no longer referenced anywhere in this test file (only defined). Consider removing it to reduce dead code, or switch call sites to use it instead of open-coding initGitRepo + &git.Client{} where appropriate.
See below for a potential fix:
Remove the now-unused newTestGitClient helper that was left behind after refactoring tests to use initGitRepo directly. This fixes the golangci-lint 'unused' error in CI. Co-authored-by: BagToad <47394200+BagToad@users.noreply.github.com> Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
BagToad
approved these changes
Apr 15, 2026
Member
BagToad
left a comment
There was a problem hiding this comment.
LGTM
I thought: maybe there's a way to fold the standalone test into the table test - but I don't think it's worth the effort to rework the table test to accommodate making multiple git dirs and whatever else it needs.
Contributor
Author
|
Good question @BagToad you are likely right it's both possible but maybe not necessarily desirable. LMK if you want it as a follow up. I will merge for sake of getting ready for release. |
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.
Summary
Fixes a bug where
gh skill publish /path/to/repo-barrun from insiderepo-foowould detectrepo-foo's remotes instead ofrepo-bar's, potentially creating a release on the wrong repository.Bug
detectGitHubRemoteused the factory-providedgitClientdirectly, which has itsRepoDirset to the current working directory. When adirargument is passed topublish, the function should look at the remotes of the target directory, not the CWD.Reproduction scenario
repo-fooworking directorygh skill publish /path/to/repo-barrepo-bar's local files, but the release gets created onrepo-fooFix
detectGitHubRemotenow accepts adirparameter, copies the git client, and setsRepoDir = dir— matching the pattern already used bydetectMissingRepoDiagnosticandcheckInstalledSkillDirs.Testing (TDD)
TestDetectGitHubRemote_UsesDir— unit test for the function directly: creates two git repos with different remotes, verifies the correct one is returnedTestPublishRun_DirArgUsesTargetRemote— integration test throughpublishRun: simulates the full bug scenario with HTTP mocks that would fail if the wrong repo is detectedskills-publish-dir-remote.txtar) — end-to-end test creating two real GitHub repos and verifying the release lands on the correct oneAll existing tests updated to initialize git repos in the skill directory (matching production behavior).
Co-authored-by: BagToad 47394200+BagToad@users.noreply.github.com