From e559a7cd5bf7789460f3fb4291524cb71b504fb4 Mon Sep 17 00:00:00 2001 From: Sam Morrow Date: Wed, 15 Apr 2026 23:05:11 +0200 Subject: [PATCH] feat(skills): auto-push unpushed commits before publish Like gh pr create, skill publish now automatically pushes unpushed local commits before creating a release. This prevents the footgun where a release is created against stale remote state when the user has local commits that haven't been pushed yet. The ensurePushed function checks for unpushed commits using git rev-list @{push}..HEAD. If commits exist or the branch has never been pushed, it pushes automatically and prints a status message. This matches the CLI's opinionated-defaults philosophy of doing the right thing by default. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- pkg/cmd/skills/publish/publish.go | 88 +++++++++++--- pkg/cmd/skills/publish/publish_test.go | 154 ++++++++++++++++++++++++- 2 files changed, 227 insertions(+), 15 deletions(-) diff --git a/pkg/cmd/skills/publish/publish.go b/pkg/cmd/skills/publish/publish.go index 931b9085c..85a3b68b9 100644 --- a/pkg/cmd/skills/publish/publish.go +++ b/pkg/cmd/skills/publish/publish.go @@ -10,6 +10,7 @@ import ( "path/filepath" "regexp" "sort" + "strconv" "strings" "github.com/MakeNowJust/heredoc" @@ -343,14 +344,14 @@ func publishRun(opts *PublishOptions) error { } owner, repo := "", "" if repoInfo != nil { - owner = repoInfo.RepoOwner() - repo = repoInfo.RepoName() + owner = repoInfo.Repo.RepoOwner() + repo = repoInfo.Repo.RepoName() } hasTopic := false var existingTags []tagEntry if owner != "" && repo != "" { if host == "" && repoInfo != nil { - host = repoInfo.RepoHost() + host = repoInfo.Repo.RepoHost() } if host != "" { if err := source.ValidateSupportedHost(host); err != nil { @@ -438,7 +439,7 @@ func publishRun(opts *PublishOptions) error { fmt.Fprintf(opts.IO.ErrOut, "\nPublishing to %s/%s...\n\n", owner, repo) - return runPublishRelease(opts, client, host, owner, repo, dir, hasTopic, existingTags) + return runPublishRelease(opts, client, host, owner, repo, dir, repoInfo.RemoteName, hasTopic, existingTags) } // repoHasTopic checks whether the repo has the agent-skills topic. @@ -473,11 +474,11 @@ func fetchTags(client *api.Client, host, owner, repo string) []tagEntry { } // runPublishRelease handles the interactive publish flow: topic, tag, release, immutability. -func runPublishRelease(opts *PublishOptions, client *api.Client, host, owner, repo, dir string, hasTopic bool, existingTags []tagEntry) error { +func runPublishRelease(opts *PublishOptions, client *api.Client, host, owner, repo, dir, remoteName string, hasTopic bool, existingTags []tagEntry) error { cs := opts.IO.ColorScheme() canPrompt := opts.IO.CanPrompt() - // 1. Add topic if missing + // Add topic if missing if !hasTopic { addTopic := true if canPrompt { @@ -498,7 +499,12 @@ func runPublishRelease(opts *PublishOptions, client *api.Client, host, owner, re } } - // 2. Determine tag + // Push unpushed commits (like gh pr create) + if err := ensurePushed(opts, dir, remoteName); err != nil { + return err + } + + // Determine tag tag := opts.Tag if tag == "" { suggested := "v1.0.0" @@ -549,7 +555,7 @@ func runPublishRelease(opts *PublishOptions, client *api.Client, host, owner, re } } - // 3. Offer to enable immutable releases + // Offer to enable immutable releases immutableEnabled := checkImmutableReleases(client, host, owner, repo) if !immutableEnabled && canPrompt { enableImmutable, err := opts.Prompter.Confirm( @@ -567,7 +573,7 @@ func runPublishRelease(opts *PublishOptions, client *api.Client, host, owner, re } } - // 4. Inform if not on default branch + // Inform if not on default branch var currentBranch string if opts.GitClient != nil { branchGitClient := opts.GitClient.Copy() @@ -581,7 +587,7 @@ func runPublishRelease(opts *PublishOptions, client *api.Client, host, owner, re fmt.Fprintf(opts.IO.ErrOut, "%s Publishing from branch %q (default is %q)\n", cs.WarningIcon(), currentBranch, defaultBranch) } - // 5. Confirm and create release + // Confirm and create release if canPrompt { confirmed, err := opts.Prompter.Confirm( fmt.Sprintf("Create release %s with auto-generated notes?", tag), true) @@ -622,6 +628,56 @@ func runPublishRelease(opts *PublishOptions, client *api.Client, host, owner, re return nil } +// ensurePushed checks whether the current branch has unpushed commits and +// pushes them automatically, consistent with how gh pr create behaves. +func ensurePushed(opts *PublishOptions, dir, remoteName string) error { + if opts.GitClient == nil { + return nil + } + + cs := opts.IO.ColorScheme() + gitClient := opts.GitClient.Copy() + gitClient.RepoDir = dir + + ctx := context.Background() + currentBranch, err := gitClient.CurrentBranch(ctx) + if err != nil { + return nil //nolint:nilerr // not on a branch (detached HEAD); skip push check + } + + // Count commits ahead of the push target (remote tracking branch). + // If the branch has no upstream, rev-list will fail; we treat that as + // "everything is unpushed" and push the whole branch. + unpushed := 0 + revCmd, err := gitClient.Command(ctx, "rev-list", "--count", "@{push}..HEAD") + if err != nil { + return fmt.Errorf("could not check unpushed commits: %w", err) + } + out, revErr := revCmd.Output() + if revErr != nil { + // @{push} not resolvable; branch has never been pushed + unpushed = -1 + } else { + n, parseErr := strconv.Atoi(strings.TrimSpace(string(out))) + if parseErr != nil { + return fmt.Errorf("could not parse unpushed commit count: %w", parseErr) + } + unpushed = n + } + + if unpushed == 0 { + return nil + } + + ref := fmt.Sprintf("HEAD:refs/heads/%s", currentBranch) + fmt.Fprintf(opts.IO.ErrOut, "%s Pushing %s to %s\n", cs.SuccessIcon(), currentBranch, remoteName) + if err := gitClient.Push(ctx, remoteName, ref); err != nil { + return fmt.Errorf("failed to push branch %s: %w", currentBranch, err) + } + + return nil +} + // detectDefaultBranch returns the default branch of the remote repo via the API. func detectDefaultBranch(client *api.Client, host, owner, repo string) string { if client == nil { @@ -866,9 +922,15 @@ func suggestNextTag(latest string) string { return fmt.Sprintf("%s%s.%s.%d", prefix, major, minor, patch+1) } +// gitHubRemote holds a detected GitHub remote and its local name. +type gitHubRemote struct { + Repo ghrepo.Interface + RemoteName string +} + // detectGitHubRemote attempts to detect the GitHub owner/repo from git remotes // in the given directory. -func detectGitHubRemote(gitClient *git.Client, dir string) (ghrepo.Interface, error) { +func detectGitHubRemote(gitClient *git.Client, dir string) (*gitHubRemote, error) { if gitClient == nil { return nil, nil } @@ -883,7 +945,7 @@ func detectGitHubRemote(gitClient *git.Client, dir string) (ghrepo.Interface, er return nil, parseErr } if repo != nil { - return repo, nil + return &gitHubRemote{Repo: repo, RemoteName: "origin"}, nil } } @@ -902,7 +964,7 @@ func detectGitHubRemote(gitClient *git.Client, dir string) (ghrepo.Interface, er return nil, parseErr } if repo != nil { - return repo, nil + return &gitHubRemote{Repo: repo, RemoteName: r.Name}, nil } } } diff --git a/pkg/cmd/skills/publish/publish_test.go b/pkg/cmd/skills/publish/publish_test.go index 6da0e01a4..82164a96a 100644 --- a/pkg/cmd/skills/publish/publish_test.go +++ b/pkg/cmd/skills/publish/publish_test.go @@ -6,6 +6,7 @@ import ( "os" "os/exec" "path/filepath" + "strings" "testing" "github.com/MakeNowJust/heredoc" @@ -22,13 +23,28 @@ import ( // initGitRepo initializes a git repo in the given directory and adds remotes. // Use this when the git repo must live in the same directory as the skill files. +// A local bare repo is created as the push target so that ensurePushed can work +// during publish tests, while the fetch URL remains the GitHub URL so that +// detectGitHubRemote still resolves the correct owner/repo. func initGitRepo(t *testing.T, dir string, remoteURLs map[string]string) { t.Helper() + + bareDir := filepath.Join(t.TempDir(), "upstream.git") + require.NoError(t, os.MkdirAll(bareDir, 0o755)) + runGitInDir(t, bareDir, "init", "--bare", "--initial-branch=main") + runGitInDir(t, dir, "init", "--initial-branch=main") runGitInDir(t, dir, "config", "user.email", "monalisa@github.com") runGitInDir(t, dir, "config", "user.name", "Monalisa Octocat") for name, url := range remoteURLs { runGitInDir(t, dir, "remote", "add", name, url) + runGitInDir(t, dir, "remote", "set-url", "--push", name, bareDir) + } + + runGitInDir(t, dir, "add", ".") + runGitInDir(t, dir, "commit", "--allow-empty", "-m", "init") + if _, ok := remoteURLs["origin"]; ok { + runGitInDir(t, dir, "push", "origin", "main") } } @@ -1392,8 +1408,8 @@ func TestDetectGitHubRemote_UsesDir(t *testing.T) { repo, err := detectGitHubRemote(gitClient, targetRepo) require.NoError(t, err) require.NotNil(t, repo) - assert.Equal(t, "monalisa", repo.RepoOwner()) - assert.Equal(t, "target-repo", repo.RepoName()) + assert.Equal(t, "monalisa", repo.Repo.RepoOwner()) + assert.Equal(t, "target-repo", repo.Repo.RepoName()) } func TestPublishRun_DirArgUsesTargetRemote(t *testing.T) { @@ -1467,3 +1483,137 @@ func runGitInDir(t *testing.T, dir string, args ...string) { out, err := cmd.CombinedOutput() require.NoError(t, err, "git %v: %s", args, out) } + +// newTestGitClientWithUpstream creates a git repo with a local bare "remote" +// and an initial commit, so we can test push/rev-list behavior realistically. +// It returns the git client and the working directory path. +func newTestGitClientWithUpstream(t *testing.T) (*git.Client, string) { + t.Helper() + parentDir := t.TempDir() + bareDir := filepath.Join(parentDir, "upstream.git") + workDir := filepath.Join(parentDir, "work") + + gitEnv := append(os.Environ(), "GIT_CONFIG_NOSYSTEM=1", "HOME="+parentDir) + + run := func(dir string, args ...string) { + t.Helper() + c := exec.Command("git", append([]string{"-C", dir}, args...)...) + c.Env = gitEnv + out, err := c.CombinedOutput() + require.NoError(t, err, "git %v: %s", args, out) + } + + // Create bare upstream + require.NoError(t, os.MkdirAll(bareDir, 0o755)) + run(bareDir, "init", "--bare", "--initial-branch=main") + + // Clone into working dir + c := exec.Command("git", "clone", bareDir, workDir) + c.Env = gitEnv + out, err := c.CombinedOutput() + require.NoError(t, err, "git clone: %s", out) + + run(workDir, "config", "user.email", "monalisa@github.com") + run(workDir, "config", "user.name", "Monalisa Octocat") + + // Create initial commit and push + require.NoError(t, os.WriteFile(filepath.Join(workDir, "README.md"), []byte("# Test"), 0o644)) + run(workDir, "add", ".") + run(workDir, "commit", "-m", "initial commit") + run(workDir, "push", "origin", "main") + + return &git.Client{ + RepoDir: workDir, + GitPath: "git", + Stderr: &bytes.Buffer{}, + Stdin: &bytes.Buffer{}, + Stdout: &bytes.Buffer{}, + }, workDir +} + +func TestEnsurePushed(t *testing.T) { + tests := []struct { + name string + setup func(t *testing.T, workDir string) + verify func(t *testing.T, workDir string) + wantErr string + wantStderr string + }{ + { + name: "no unpushed commits is a no-op", + setup: func(_ *testing.T, _ string) { + // initial commit already pushed by helper + }, + }, + { + name: "unpushed commits are pushed automatically", + setup: func(t *testing.T, workDir string) { + t.Helper() + require.NoError(t, os.WriteFile(filepath.Join(workDir, "new.txt"), []byte("new"), 0o644)) + runGitInDir(t, workDir, "add", ".") + runGitInDir(t, workDir, "commit", "-m", "unpushed change") + }, + verify: func(t *testing.T, workDir string) { + t.Helper() + // After push, rev-list should show 0 unpushed commits + cmd := exec.Command("git", "-C", workDir, "rev-list", "--count", "@{push}..HEAD") + cmd.Env = append(os.Environ(), "GIT_CONFIG_NOSYSTEM=1", "HOME="+workDir) + out, err := cmd.CombinedOutput() + require.NoError(t, err, "rev-list: %s", out) + assert.Equal(t, "0", strings.TrimSpace(string(out))) + }, + wantStderr: "Pushing main to origin", + }, + { + name: "new branch never pushed is pushed automatically", + setup: func(t *testing.T, workDir string) { + t.Helper() + runGitInDir(t, workDir, "checkout", "-b", "feature") + require.NoError(t, os.WriteFile(filepath.Join(workDir, "feat.txt"), []byte("feat"), 0o644)) + runGitInDir(t, workDir, "add", ".") + runGitInDir(t, workDir, "commit", "-m", "new branch commit") + }, + verify: func(t *testing.T, workDir string) { + t.Helper() + // After push, the branch should exist on the remote + cmd := exec.Command("git", "-C", workDir, "rev-list", "--count", "@{push}..HEAD") + cmd.Env = append(os.Environ(), "GIT_CONFIG_NOSYSTEM=1", "HOME="+workDir) + out, err := cmd.CombinedOutput() + require.NoError(t, err, "rev-list: %s", out) + assert.Equal(t, "0", strings.TrimSpace(string(out))) + }, + wantStderr: "Pushing feature to origin", + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + gitClient, workDir := newTestGitClientWithUpstream(t) + tt.setup(t, workDir) + + ios, _, _, stderr := iostreams.Test() + ios.SetStdoutTTY(true) + ios.SetStderrTTY(true) + + opts := &PublishOptions{ + IO: ios, + GitClient: gitClient, + } + + err := ensurePushed(opts, workDir, "origin") + + if tt.wantErr != "" { + require.Error(t, err) + assert.Contains(t, err.Error(), tt.wantErr) + } else { + require.NoError(t, err) + } + if tt.wantStderr != "" { + assert.Contains(t, stderr.String(), tt.wantStderr) + } + if tt.verify != nil { + tt.verify(t, workDir) + } + }) + } +}