Skills: replace real git in publish tests with CommandStubber by SamMorrowDrums · Pull Request #13188 · cli/cli

SamMorrowDrums · 2026-04-16T14:45:36Z

Summary

Follow-up from #13184 / PR #13185. Replaces all real exec.Command("git", ...) usage in pkg/cmd/skills/publish/publish_test.go with run.Stub() / run.CommandStubber, as requested in PR #13171 review feedback.

Changes

Removed helpers: initGitRepo (created real git repos with bare remotes), runGitInDir, newTestGitClientWithUpstream
Added helpers: stubGitRemote (stubs git remote -v, git config --get-regexp, git remote get-url) and stubEnsurePushed (stubs git symbolic-ref, git rev-list, second CurrentBranch call)
Converted 20+ test cases in TestPublishRun to use command stubs instead of real git
Converted TestEnsurePushed to verify correct git commands via stubs
Converted TestDetectGitHubRemote_UsesDir and TestPublishRun_DirArgUsesTargetRemote** to use stubs
Removed imports: os/exec, strings; Added: internal/run

Notes

The installed_skill_dirs_not_gitignored_warns test expectation was adjusted: with stubs, IsIgnored can't distinguish exit code 1 (not ignored) from other errors because errWithExitCode doesn't satisfy *exec.ExitError. The test still validates the gitignore warning is shown.
All patterns use git( .+)? prefix to match optional -C <dir> arguments injected by git.Client.

Fixes #13186

babakks

Thanks for the fix! 🙏

All seems good, except for how we create the mock git client. See my comment below.

babakks · 2026-04-16T15:01:41Z

+// for each remote in the map. The map key is the remote name and the value is the URL.
+// Patterns use `git( .+)?` prefix to match optional `-C <dir>` arguments.
+func stubGitRemote(cs *run.CommandStubber, remoteURLs map[string]string) {
+	// Build `git remote -v` output


nitpick: redundant comment (just the // Build ... line).

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

 	for name, url := range remoteURLs {
-		runGitInDir(t, dir, "remote", "add", name, url)
-		runGitInDir(t, dir, "remote", "set-url", "--push", name, bareDir)
+		remoteLines += fmt.Sprintf("%s\t%s (fetch)\n%s\t%s (push)\n", name, url, name, url)


nitpick: can reuse args like this:

remoteLines += fmt.Sprintf("%[1]s\t%[2]s (fetch)\n%[1]s\t%[2]s (push)\n", name, url)

babakks · 2026-04-16T15:05:00Z

+	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`, name), 0, url+"\n")


nitpick: name should be regexp-escaped:

cs.Register(fmt.Sprintf(`git( .+)? remote get-url -- %s`, regexp.QuoteMeta(name)), 0, url+"\n")

babakks · 2026-04-16T15:13:30Z

-				assert.Equal(t, "0", strings.TrimSpace(string(out)))
+			cmdStubs: func(cs *run.CommandStubber) {
+				cs.Register(`git( .+)? symbolic-ref --quiet HEAD`, 0, "refs/heads/feature\n")
+				// rev-list fails when branch has no upstream


s/has no upstream/is not pushed/

babakks · 2026-04-16T15:17:39Z

-		GitClient: &git.Client{},
+		IO:         ios,
+		Dir:        dir,
+		GitClient:  &git.Client{},


We should initialise git.Client like this (otherwise it'll go through finding the real git path, see here and here):

&git.Client{ GhPath: "some/path/gh", GitPath: "some/path/git", },

Maybe do it via a helper function, or if all test cases need a git client (which they probably do), create the git client in the test loop.

babakks

I approve to prevent the blocker once we're merging this.

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>

SamMorrowDrums · 2026-04-16T17:06:06Z

Rebased and addressed all review feedback:

Removed redundant // Build ... comment in stubGitRemote
Using %[1]s/%[2]s format args for the remote lines sprintf
Using regexp.QuoteMeta(name) for the get-url pattern
Fixed comment: "is not pushed" instead of "has no upstream"
Added newTestGitClient() helper returning &git.Client{GitPath: "some/path/git"} — used everywhere to avoid real git path resolution

All tests pass, lint clean.

…h-tests

Copilot

Pull request overview

Replaces real git invocations in skills publish tests with internal/run command stubs to make the test suite hermetic and consistent with project patterns.

Changes:

Removed helper utilities that created and manipulated real git repositories via exec.Command("git", ...).
Introduced run.Stub()/run.CommandStubber-based helpers to stub git remote detection and push-check behavior.
Converted TestPublishRun, TestEnsurePushed, and remote-selection tests to validate behavior through expected command invocations.

Show a summary per file

File	Description
`pkg/cmd/skills/publish/publish_test.go`	Reworks publish/push/remote-related tests to stub git commands via `run.CommandStubber` instead of executing real git

Copilot's findings

Tip

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

Comments suppressed due to low confidence (1)

pkg/cmd/skills/publish/publish_test.go:1583

Same issue as the dedicated remote-detection test: the stubs here don't assert which directory git commands are executed against, so this regression test won't catch a bug where publishRun mistakenly detects remotes from the factory client's RepoDir (cwdRepo) instead of the Dir argument (targetRepo). Consider making the git stubs directory-specific (or asserting on -C <targetRepo> via a stub callback) so the test meaningfully verifies the behavior it describes.

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

	writeSkill(t, targetRepo, "my-skill", heredoc.Doc(`
		---
		name: my-skill
		description: A test skill
		license: MIT
		---
		Body text.
	`))

	ios, _, stdout, _ := iostreams.Test()
	ios.SetStdoutTTY(true)
	ios.SetStdinTTY(true)
	ios.SetStderrTTY(true)

	reg := &httpmock.Registry{}
	defer reg.Verify(t)
	stubAllSecureRemote(reg, "monalisa", "target-repo")

	err := publishRun(&PublishOptions{
		IO:         ios,
		Dir:        targetRepo,
		DryRun:     true,
		GitClient:  &git.Client{GitPath: "some/path/git", RepoDir: cwdRepo},
		HttpClient: func() (*http.Client, error) { return &http.Client{Transport: reg}, nil },

Files reviewed: 1/1 changed files
Comments generated: 5

Copilot · 2026-04-16T17:12:24Z

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


This test is meant to verify that detectGitHubRemote uses the provided dir rather than the git client's existing RepoDir, but the current stubs don't distinguish between -C <cwdRepo> and -C <targetRepo> (the regexes match either). As a result, the test would still pass even if the implementation mistakenly used cwdRepo. Consider tightening the stubs (e.g., match on the expected -C <targetRepo> argument or add a callback assertion on the invoked args) so the test actually fails when the wrong directory is used.

This issue also appears on line 1551 of the same file.

Copilot · 2026-04-16T17:12:25Z

 					HttpClient: func() (*http.Client, error) { return &http.Client{Transport: reg}, nil },
 					host:       "github.com",
 				}
 			},


In the "publish adds missing topic via --tag" test case, the assertions for user-facing output appear to have been removed (the case no longer sets wantStdout/wantStderr). This makes the test less effective at verifying that the topic-add message and publish output are actually emitted. Consider restoring an assertion for the expected output (e.g., that the "Added "agent-skills" topic" and/or "Published" messages are printed).

Suggested change

},

},

wantStdout: "Added \"agent-skills\" topic\nPublished v1.0.0",

Copilot · 2026-04-16T17:12:25Z

+				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")


This cmd stub setup duplicates the same git remote -v / git config --get-regexp ... / git remote get-url registrations that stubGitRemote already provides, just to satisfy the second Remotes() call from detectMissingRepoDiagnostic. To reduce duplication and make the intent clearer, consider reusing stubGitRemote for the second call as well (e.g., by calling it twice or by introducing a small helper that registers the remote stubs N times).

Suggested change

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

stubGitRemote(cs, map[string]string{

"origin": "https://gitlab.com/hubot/bar.git",

})

Copilot · 2026-04-16T17:12:25Z

+			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, "")


Within this test case, the stubbed git check-ignore exit status 1 is treated as an error (because CommandStubber's error doesn't satisfy *exec.ExitError), so the behavior under test is effectively the "could not verify" warning path rather than the true "not gitignored" path. Consider renaming the case accordingly (or adjusting the stubbing approach) so the test name matches what it's validating.

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

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

SamMorrowDrums requested review from BagToad and Copilot and removed request for Copilot April 16, 2026 14:45

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

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

SamMorrowDrums changed the base branch from trunk to sammorrowdrums/skills-post-merge-review-followups April 16, 2026 15:07

SamMorrowDrums changed the base branch from sammorrowdrums/skills-post-merge-review-followups to trunk April 16, 2026 15:10

SamMorrowDrums changed the base branch from trunk to sammorrowdrums/skills-post-merge-review-followups April 16, 2026 15:11

SamMorrowDrums force-pushed the sammorrowdrums/skills-replace-git-in-publish-tests branch from 5b5a1f7 to 0eb250a Compare April 16, 2026 15:14

babakks requested changes Apr 16, 2026

View reviewed changes

babakks approved these changes Apr 16, 2026

View reviewed changes

Base automatically changed from sammorrowdrums/skills-post-merge-review-followups to trunk April 16, 2026 16:59

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

Copilot AI review requested due to automatic review settings April 16, 2026 17:03

SamMorrowDrums force-pushed the sammorrowdrums/skills-replace-git-in-publish-tests branch from 0eb250a to 5ae5d1d Compare April 16, 2026 17:03

Copilot started reviewing on behalf of SamMorrowDrums April 16, 2026 17:04 View session

Merge branch 'trunk' into sammorrowdrums/skills-replace-git-in-publis…

bce04e3

…h-tests

SamMorrowDrums enabled auto-merge April 16, 2026 17:06

SamMorrowDrums disabled auto-merge April 16, 2026 17:09

Copilot AI reviewed Apr 16, 2026

View reviewed changes

Update publish_test.go

c5c7d79

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

babakks approved these changes Apr 16, 2026

View reviewed changes

babakks enabled auto-merge April 16, 2026 17:28

babakks merged commit 26d2302 into trunk Apr 16, 2026
11 checks passed

babakks deleted the sammorrowdrums/skills-replace-git-in-publish-tests branch April 16, 2026 17:30

	},
	},
	wantStdout: "Added \"agent-skills\" topic\nPublished v1.0.0",

Conversation

SamMorrowDrums commented Apr 16, 2026

Summary

Changes

Notes

Uh oh!

babakks left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

babakks Apr 16, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

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!

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!

babakks left a comment

Choose a reason for hiding this comment

Uh oh!

SamMorrowDrums commented Apr 16, 2026

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Copilot's findings

Uh oh!

Copilot AI Apr 16, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Apr 16, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Apr 16, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Apr 16, 2026

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

babakks left a comment •

edited

Loading

babakks Apr 16, 2026 •

edited

Loading