From 39cfc2e4e554dcbfcc26a5c03bf3f16d97fe17db Mon Sep 17 00:00:00 2001 From: euxaristia <25621994+euxaristia@users.noreply.github.com> Date: Tue, 14 Jul 2026 06:27:34 -0400 Subject: [PATCH 1/5] feat(zerogit): auto-create a conventional branch before push/pr on default branch zero changes push/pr refused to push straight to the default branch but never offered an alternative, unlike other agent tooling that names and checks out a feature branch automatically. Add CreateBranch, IsDefaultBranch, CurrentGitUser, and branch-name slugify/build helpers to zerogit, and wire push/pr to auto-create a "/" branch (slug from an LLM when a provider is configured, otherwise a deterministic fallback from the diff) whenever the current branch is the default one and --yes/--dry-run weren't passed. --- internal/cli/app.go | 27 ++++- internal/cli/workflow_test.go | 175 +++++++++++++++++++++++++++++++ internal/cli/workflows.go | 110 +++++++++++++++++++ internal/zerogit/zerogit.go | 138 ++++++++++++++++++++++++ internal/zerogit/zerogit_test.go | 148 ++++++++++++++++++++++++++ 5 files changed, 593 insertions(+), 5 deletions(-) diff --git a/internal/cli/app.go b/internal/cli/app.go index a138f0142..2b0b7f28f 100644 --- a/internal/cli/app.go +++ b/internal/cli/app.go @@ -83,6 +83,9 @@ type appDeps struct { commitChanges func(context.Context, zerogit.CommitOptions) (zerogit.CommitResult, error) pushChanges func(context.Context, zerogit.PushOptions) (zerogit.PushResult, error) createPR func(context.Context, zerogit.PROptions) (zerogit.PRResult, error) + createBranch func(context.Context, zerogit.BranchOptions) (zerogit.BranchResult, error) + isDefaultBranch func(context.Context, zerogit.DefaultBranchOptions) (bool, string, error) + currentGitUser func(context.Context, string) string runTUI func(context.Context, tui.Options) int runEditor func(string) error checkUpdate func(context.Context, update.Options) (update.Result, error) @@ -188,11 +191,16 @@ func defaultAppDeps() appDeps { commitChanges: zerogit.Commit, pushChanges: zerogit.Push, createPR: zerogit.CreatePR, - runTUI: tui.Run, - runEditor: openEditor, - checkUpdate: update.Check, - applyUpdate: update.Apply, - now: time.Now, + createBranch: zerogit.CreateBranch, + isDefaultBranch: zerogit.IsDefaultBranch, + currentGitUser: func(ctx context.Context, cwd string) string { + return zerogit.CurrentGitUser(ctx, cwd, nil) + }, + runTUI: tui.Run, + runEditor: openEditor, + checkUpdate: update.Check, + applyUpdate: update.Apply, + now: time.Now, } } @@ -537,6 +545,15 @@ func fillAppDeps(deps appDeps) appDeps { if deps.createPR == nil { deps.createPR = defaults.createPR } + if deps.createBranch == nil { + deps.createBranch = defaults.createBranch + } + if deps.isDefaultBranch == nil { + deps.isDefaultBranch = defaults.isDefaultBranch + } + if deps.currentGitUser == nil { + deps.currentGitUser = defaults.currentGitUser + } if deps.runTUI == nil { deps.runTUI = defaults.runTUI } diff --git a/internal/cli/workflow_test.go b/internal/cli/workflow_test.go index 75b844f47..8be4a1b54 100644 --- a/internal/cli/workflow_test.go +++ b/internal/cli/workflow_test.go @@ -803,3 +803,178 @@ func TestRunChangesCommitAuto(t *testing.T) { } }) } + +func TestEnsureFeatureBranchCreatesBranchOffDefaultWithoutProvider(t *testing.T) { + cwd := t.TempDir() + var createdName string + + branch, err := ensureFeatureBranch(context.Background(), &bytes.Buffer{}, false, cwd, false, false, appDeps{ + isDefaultBranch: func(ctx context.Context, options zerogit.DefaultBranchOptions) (bool, string, error) { + return true, "main", nil + }, + inspectChanges: func(ctx context.Context, options zerogit.InspectOptions) (zerogit.ChangeSummary, error) { + return zerogit.ChangeSummary{Files: []zerogit.FileChange{{Path: "README.md", Status: "modified"}}}, nil + }, + resolveConfig: func(workspaceRoot string, overrides config.Overrides) (config.ResolvedConfig, error) { + return config.ResolvedConfig{}, nil + }, + currentGitUser: func(ctx context.Context, cwd string) string { return "Someone" }, + createBranch: func(ctx context.Context, options zerogit.BranchOptions) (zerogit.BranchResult, error) { + createdName = options.Name + return zerogit.BranchResult{Branch: options.Name}, nil + }, + }) + if err != nil { + t.Fatalf("ensureFeatureBranch returned error: %v", err) + } + if branch != "someone/readme-md" || createdName != "someone/readme-md" { + t.Fatalf("unexpected branch: got %q (created %q)", branch, createdName) + } +} + +func TestEnsureFeatureBranchUsesLLMSlugWhenProviderConfigured(t *testing.T) { + cwd := t.TempDir() + mockProv := &mockCommitMsgProvider{response: "add login page"} + var createdName string + + branch, err := ensureFeatureBranch(context.Background(), &bytes.Buffer{}, false, cwd, false, false, appDeps{ + isDefaultBranch: func(ctx context.Context, options zerogit.DefaultBranchOptions) (bool, string, error) { + return true, "main", nil + }, + inspectChanges: func(ctx context.Context, options zerogit.InspectOptions) (zerogit.ChangeSummary, error) { + return zerogit.ChangeSummary{Files: []zerogit.FileChange{{Path: "login.go", Status: "added"}}, Diff: "+func Login() {}"}, nil + }, + resolveConfig: func(workspaceRoot string, overrides config.Overrides) (config.ResolvedConfig, error) { + return execResolvedConfig(), nil + }, + newProvider: func(profile config.ProviderProfile) (zeroruntime.Provider, error) { + return mockProv, nil + }, + currentGitUser: func(ctx context.Context, cwd string) string { return "Someone" }, + createBranch: func(ctx context.Context, options zerogit.BranchOptions) (zerogit.BranchResult, error) { + createdName = options.Name + return zerogit.BranchResult{Branch: options.Name}, nil + }, + }) + if err != nil { + t.Fatalf("ensureFeatureBranch returned error: %v", err) + } + if branch != "someone/add-login-page" || createdName != "someone/add-login-page" { + t.Fatalf("unexpected branch: got %q (created %q)", branch, createdName) + } +} + +func TestEnsureFeatureBranchSkipsWhenNotOnDefault(t *testing.T) { + cwd := t.TempDir() + createBranchCalled := false + + branch, err := ensureFeatureBranch(context.Background(), &bytes.Buffer{}, false, cwd, false, false, appDeps{ + isDefaultBranch: func(ctx context.Context, options zerogit.DefaultBranchOptions) (bool, string, error) { + return false, "feat/existing", nil + }, + createBranch: func(ctx context.Context, options zerogit.BranchOptions) (zerogit.BranchResult, error) { + createBranchCalled = true + return zerogit.BranchResult{}, nil + }, + }) + if err != nil { + t.Fatalf("ensureFeatureBranch returned error: %v", err) + } + if branch != "feat/existing" { + t.Fatalf("expected existing branch to be returned unchanged, got %q", branch) + } + if createBranchCalled { + t.Fatal("expected createBranch not to be called when already off the default branch") + } +} + +func TestEnsureFeatureBranchSkipsWhenAllowDefaultOrDryRun(t *testing.T) { + for _, tc := range []struct { + name string + allowDefaultBranch bool + dryRun bool + }{ + {"AllowDefaultBranch", true, false}, + {"DryRun", false, true}, + } { + t.Run(tc.name, func(t *testing.T) { + cwd := t.TempDir() + branch, err := ensureFeatureBranch(context.Background(), &bytes.Buffer{}, false, cwd, tc.allowDefaultBranch, tc.dryRun, appDeps{ + isDefaultBranch: func(ctx context.Context, options zerogit.DefaultBranchOptions) (bool, string, error) { + t.Fatal("isDefaultBranch should not be called") + return false, "", nil + }, + }) + if err != nil { + t.Fatalf("ensureFeatureBranch returned error: %v", err) + } + if branch != "" { + t.Fatalf("expected empty branch (defer to current HEAD), got %q", branch) + } + }) + } +} + +func TestRunChangesPushCreatesFeatureBranchWhenOnDefault(t *testing.T) { + cwd := t.TempDir() + var pushedBranch string + + var stdout, stderr bytes.Buffer + exitCode := runWithDeps([]string{"changes", "push"}, &stdout, &stderr, appDeps{ + getwd: func() (string, error) { return cwd, nil }, + isDefaultBranch: func(ctx context.Context, options zerogit.DefaultBranchOptions) (bool, string, error) { + return true, "main", nil + }, + inspectChanges: func(ctx context.Context, options zerogit.InspectOptions) (zerogit.ChangeSummary, error) { + return zerogit.ChangeSummary{Files: []zerogit.FileChange{{Path: "README.md", Status: "modified"}}}, nil + }, + resolveConfig: func(workspaceRoot string, overrides config.Overrides) (config.ResolvedConfig, error) { + return config.ResolvedConfig{}, nil + }, + currentGitUser: func(ctx context.Context, cwd string) string { return "Someone" }, + createBranch: func(ctx context.Context, options zerogit.BranchOptions) (zerogit.BranchResult, error) { + return zerogit.BranchResult{Branch: options.Name}, nil + }, + pushChanges: func(ctx context.Context, options zerogit.PushOptions) (zerogit.PushResult, error) { + pushedBranch = options.Branch + return zerogit.PushResult{Remote: "origin", Branch: options.Branch}, nil + }, + }) + + if exitCode != exitSuccess { + t.Fatalf("expected exit code %d, got %d: %s", exitSuccess, exitCode, stderr.String()) + } + if pushedBranch != "someone/readme-md" { + t.Fatalf("expected push to target the newly created branch, got %q", pushedBranch) + } + if !strings.Contains(stdout.String(), "Created branch someone/readme-md") { + t.Fatalf("expected branch-creation message in stdout, got %q", stdout.String()) + } +} + +func TestRunChangesPushSkipsBranchCreationWithYes(t *testing.T) { + cwd := t.TempDir() + isDefaultBranchCalled := false + + var stdout, stderr bytes.Buffer + exitCode := runWithDeps([]string{"changes", "push", "--yes"}, &stdout, &stderr, appDeps{ + getwd: func() (string, error) { return cwd, nil }, + isDefaultBranch: func(ctx context.Context, options zerogit.DefaultBranchOptions) (bool, string, error) { + isDefaultBranchCalled = true + return true, "main", nil + }, + pushChanges: func(ctx context.Context, options zerogit.PushOptions) (zerogit.PushResult, error) { + if options.Branch != "" { + t.Fatalf("expected empty Branch (defer to current HEAD) with --yes, got %q", options.Branch) + } + return zerogit.PushResult{Remote: "origin", Branch: "main"}, nil + }, + }) + + if exitCode != exitSuccess { + t.Fatalf("expected exit code %d, got %d: %s", exitSuccess, exitCode, stderr.String()) + } + if isDefaultBranchCalled { + t.Fatal("expected isDefaultBranch not to be consulted when --yes is passed") + } +} diff --git a/internal/cli/workflows.go b/internal/cli/workflows.go index bc13bc7f8..613d6546a 100644 --- a/internal/cli/workflows.go +++ b/internal/cli/workflows.go @@ -4,6 +4,7 @@ import ( "context" "fmt" "io" + "path/filepath" "strconv" "strings" "time" @@ -862,9 +863,15 @@ func runChangesPush(args []string, stdout io.Writer, stderr io.Writer, deps appD return writeExecUsageError(stderr, err.Error()) } + branch, err := ensureFeatureBranch(context.Background(), stdout, options.json, workspaceRoot, options.yes, options.dryRun, deps) + if err != nil { + return writeExecUsageError(stderr, err.Error()) + } + result, err := deps.pushChanges(context.Background(), zerogit.PushOptions{ Cwd: workspaceRoot, Remote: options.remote, + Branch: branch, Force: options.force, DryRun: options.dryRun, AllowPushDefaultBranch: options.yes, @@ -914,6 +921,11 @@ func runChangesPR(args []string, stdout io.Writer, stderr io.Writer, deps appDep return writeExecUsageError(stderr, err.Error()) } + branch, err := ensureFeatureBranch(context.Background(), stdout, options.json, workspaceRoot, options.yes, false, deps) + if err != nil { + return writeExecUsageError(stderr, err.Error()) + } + if !options.json { if _, err := fmt.Fprintln(stdout, "Pushing current branch to set upstream..."); err != nil { return exitCrash @@ -922,6 +934,7 @@ func runChangesPR(args []string, stdout io.Writer, stderr io.Writer, deps appDep pushResult, err := deps.pushChanges(context.Background(), zerogit.PushOptions{ Cwd: workspaceRoot, Remote: options.remote, + Branch: branch, Force: options.force, AllowPushDefaultBranch: options.yes, }) @@ -1004,3 +1017,100 @@ func generateAutoCommitMessage(ctx context.Context, provider zeroruntime.Provide } return msg, nil } + +// ensureFeatureBranch is the branch-naming step `zero changes push`/`pr` run +// before pushing: pushing straight to the default branch is refused deeper in +// zerogit.Push, so rather than surface that as a dead end, create and switch +// to a conventionally named "/" branch first. It returns the +// branch push/pr should target, or "" to mean "current HEAD branch, unchanged" +// (zerogit.Push already treats an empty Branch that way). allowDefaultBranch +// (the --yes flag) and dryRun both opt out via that "" return, leaving Push's +// own guard/preview behavior on the default branch unaffected. +func ensureFeatureBranch(ctx context.Context, stdout io.Writer, jsonMode bool, workspaceRoot string, allowDefaultBranch bool, dryRun bool, deps appDeps) (string, error) { + if allowDefaultBranch || dryRun { + return "", nil + } + + isDefault, currentBranch, err := deps.isDefaultBranch(ctx, zerogit.DefaultBranchOptions{Cwd: workspaceRoot}) + if err != nil { + return "", err + } + if !isDefault { + return currentBranch, nil + } + + summary, err := deps.inspectChanges(ctx, zerogit.InspectOptions{Cwd: workspaceRoot}) + if err != nil { + return "", fmt.Errorf("failed to inspect changes: %w", err) + } + + slug := fallbackBranchSlug(summary) + if resolved, cfgErr := deps.resolveConfig(workspaceRoot, config.Overrides{}); cfgErr == nil && config.HasProviderProfile(resolved.Provider) { + if provider, provErr := deps.newProvider(resolved.Provider); provErr == nil { + if !jsonMode { + fmt.Fprintln(stdout, "Generating branch name using LLM...") + } + genCtx, cancel := context.WithTimeout(ctx, 60*time.Second) + generated, genErr := generateAutoBranchSlug(genCtx, provider, resolved.Provider.Model, redactChangeSummary(summary)) + cancel() + if genErr == nil && generated != "" { + slug = generated + } + } + } + + name := zerogit.BuildBranchName(deps.currentGitUser(ctx, workspaceRoot), slug) + result, err := deps.createBranch(ctx, zerogit.BranchOptions{Cwd: workspaceRoot, Name: name}) + if err != nil { + return "", fmt.Errorf("failed to create branch: %w", err) + } + if !jsonMode { + fmt.Fprintf(stdout, "Created branch %s (was on %s)\n", result.Branch, currentBranch) + } + return result.Branch, nil +} + +// fallbackBranchSlug derives a deterministic branch-name slug from a change +// summary without calling an LLM, so ensureFeatureBranch still works when no +// provider is configured. +func fallbackBranchSlug(summary zerogit.ChangeSummary) string { + switch len(summary.Files) { + case 0: + return "changes" + case 1: + return zerogit.SlugifyBranchComponent(filepath.Base(summary.Files[0].Path)) + default: + return fmt.Sprintf("update-%d-files", len(summary.Files)) + } +} + +// generateAutoBranchSlug asks the model for a short kebab-case slug +// describing the diff, mirroring generateAutoCommitMessage's prompt shape. +func generateAutoBranchSlug(ctx context.Context, provider zeroruntime.Provider, model string, summary zerogit.ChangeSummary) (string, error) { + var promptBuilder strings.Builder + promptBuilder.WriteString("Analyze the following git diff and generate a short git branch name slug for it.\n") + promptBuilder.WriteString("The slug must be 2 to 5 lowercase words separated by hyphens (kebab-case), using only letters, digits, and hyphens, with no prefix like \"feature/\" or \"fix/\" and no surrounding quotes.\n") + promptBuilder.WriteString("Output ONLY the raw slug text, nothing else.\n\n") + promptBuilder.WriteString("Git Diff:\n") + promptBuilder.WriteString(summary.Diff) + + request := zeroruntime.CompletionRequest{ + Messages: []zeroruntime.Message{ + {Role: zeroruntime.MessageRoleUser, Content: promptBuilder.String()}, + }, + } + stream, err := provider.StreamCompletion(ctx, request) + if err != nil { + return "", err + } + collected := zeroruntime.CollectStream(ctx, stream) + if collected.Error != "" { + return "", fmt.Errorf("%s", collected.Error) + } + + slug := zerogit.SlugifyBranchComponent(collected.Text) + if slug == "" { + return "", fmt.Errorf("provider returned empty branch slug") + } + return slug, nil +} diff --git a/internal/zerogit/zerogit.go b/internal/zerogit/zerogit.go index f7bff7c17..b49ab4790 100644 --- a/internal/zerogit/zerogit.go +++ b/internal/zerogit/zerogit.go @@ -6,7 +6,9 @@ import ( "fmt" "os" "os/exec" + "os/user" "path/filepath" + "regexp" "strings" "unicode/utf8" @@ -618,6 +620,142 @@ func isDefaultBranch(ctx context.Context, runGit Runner, dir, remote, branch str return branch == "main" || branch == "master" } +// DefaultBranchOptions resolves whether a branch is the repository's +// default/protected branch. +type DefaultBranchOptions struct { + Cwd string + Remote string + Branch string // empty resolves the current branch + RunGit Runner +} + +// IsDefaultBranch reports whether options.Branch (or, if empty, the current +// branch) is the repository's default/protected branch, using the same check +// Push already applies before refusing to push straight to it. It returns the +// resolved branch name alongside the bool so callers that left Branch empty +// don't need a second lookup. +func IsDefaultBranch(ctx context.Context, options DefaultBranchOptions) (bool, string, error) { + cwd, err := resolveCwd(options.Cwd) + if err != nil { + return false, "", err + } + runGit, _ := resolveRunners(options.RunGit, nil) + + root, err := gitOutput(ctx, runGit, cwd, "rev-parse", "--show-toplevel") + if err != nil { + return false, "", fmt.Errorf("not a git repository: %w", err) + } + root = filepath.Clean(root) + + branch := strings.TrimSpace(options.Branch) + if branch == "" { + branch, err = gitOutput(ctx, runGit, root, "rev-parse", "--abbrev-ref", "HEAD") + if err != nil { + return false, "", fmt.Errorf("resolve current branch: %w", err) + } + } + remote := strings.TrimSpace(options.Remote) + if remote == "" { + remote = "origin" + } + return isDefaultBranch(ctx, runGit, root, remote, branch), branch, nil +} + +// BranchOptions configures creating and checking out a new local branch. +type BranchOptions struct { + Cwd string + Name string // full branch name, e.g. "alice/fix-typo" + DryRun bool + RunGit Runner +} + +// BranchResult reports the branch that was (or, in dry-run, would be) created. +type BranchResult struct { + Branch string `json:"branch"` +} + +// CreateBranch checks out a new local branch named options.Name off the +// current HEAD. DryRun previews the resolved name without mutating the +// repository, matching the DryRun convention used by Commit and Push. +func CreateBranch(ctx context.Context, options BranchOptions) (BranchResult, error) { + cwd, err := resolveCwd(options.Cwd) + if err != nil { + return BranchResult{}, err + } + runGit, _ := resolveRunners(options.RunGit, nil) + + root, err := gitOutput(ctx, runGit, cwd, "rev-parse", "--show-toplevel") + if err != nil { + return BranchResult{}, fmt.Errorf("not a git repository: %w", err) + } + root = filepath.Clean(root) + + name := strings.TrimSpace(options.Name) + if name == "" { + return BranchResult{}, fmt.Errorf("branch name required") + } + if options.DryRun { + return BranchResult{Branch: name}, nil + } + if _, err := gitOutput(ctx, runGit, root, "checkout", "-b", name); err != nil { + return BranchResult{}, fmt.Errorf("create branch %q: %w", name, err) + } + return BranchResult{Branch: name}, nil +} + +// CurrentGitUser resolves an identity to prefix generated branch names with: +// git config user.name, falling back to the OS account username, falling +// back to the literal "user" so BuildBranchName always gets a non-empty +// input. +func CurrentGitUser(ctx context.Context, cwd string, runGit Runner) string { + runGit, _ = resolveRunners(runGit, nil) + if name, err := gitOutput(ctx, runGit, cwd, "config", "user.name"); err == nil && name != "" { + return name + } + if u, err := user.Current(); err == nil && u.Username != "" { + return u.Username + } + return "user" +} + +// slugComponentRe matches runs of characters not allowed in a branch-name +// component, so SlugifyBranchComponent can collapse them to a single hyphen. +var slugComponentRe = regexp.MustCompile(`[^a-z0-9]+`) + +// maxSlugComponentLen caps a single branch-name component so generated names +// stay short and readable, matching the "username/feature-name" convention +// rather than sprawling into a full sentence. +const maxSlugComponentLen = 40 + +// SlugifyBranchComponent lowercases s and collapses any run of non +// alphanumeric characters into a single hyphen, trimming leading/trailing +// hyphens and capping length so the result is a safe, short branch-name +// component. +func SlugifyBranchComponent(s string) string { + slug := slugComponentRe.ReplaceAllString(strings.ToLower(strings.TrimSpace(s)), "-") + slug = strings.Trim(slug, "-") + if len(slug) > maxSlugComponentLen { + slug = strings.Trim(slug[:maxSlugComponentLen], "-") + } + return slug +} + +// BuildBranchName composes a "/" branch name from a git identity +// and a short feature slug (the convention used across Gitlawb tooling). +// Empty or unsafe inputs fall back to "user" and "changes" respectively so +// the result is always a valid, non-empty branch name. +func BuildBranchName(gitUser, slug string) string { + userSlug := SlugifyBranchComponent(gitUser) + if userSlug == "" { + userSlug = "user" + } + featureSlug := SlugifyBranchComponent(slug) + if featureSlug == "" { + featureSlug = "changes" + } + return userSlug + "/" + featureSlug +} + type PROptions struct { Cwd string Fill bool diff --git a/internal/zerogit/zerogit_test.go b/internal/zerogit/zerogit_test.go index cfb479e6e..853e665c6 100644 --- a/internal/zerogit/zerogit_test.go +++ b/internal/zerogit/zerogit_test.go @@ -727,3 +727,151 @@ func TestCreatePRCommandConstruction(t *testing.T) { } }) } + +func TestCreateBranch(t *testing.T) { + t.Run("HappyPath", func(t *testing.T) { + root := t.TempDir() + runner := &fakeRunner{results: []CommandResult{ + {Stdout: root + "\n"}, + {Stdout: "Switched to a new branch 'alice/fix-typo'\n"}, + }} + + result, err := CreateBranch(context.Background(), BranchOptions{ + Cwd: root, + Name: "alice/fix-typo", + RunGit: runner.Run, + }) + if err != nil { + t.Fatalf("CreateBranch returned error: %v", err) + } + if result.Branch != "alice/fix-typo" { + t.Fatalf("unexpected branch: %#v", result) + } + if got := runner.commandLine(1); got != "git checkout -b alice/fix-typo" { + t.Fatalf("unexpected checkout command: %q", got) + } + }) + + t.Run("DryRunDoesNotCheckout", func(t *testing.T) { + root := t.TempDir() + runner := &fakeRunner{results: []CommandResult{ + {Stdout: root + "\n"}, + }} + + result, err := CreateBranch(context.Background(), BranchOptions{ + Cwd: root, + Name: "alice/fix-typo", + DryRun: true, + RunGit: runner.Run, + }) + if err != nil { + t.Fatalf("CreateBranch returned error: %v", err) + } + if result.Branch != "alice/fix-typo" { + t.Fatalf("unexpected branch: %#v", result) + } + if len(runner.calls) != 1 { + t.Fatalf("expected only the toplevel lookup call, got %d calls", len(runner.calls)) + } + }) + + t.Run("RequiresName", func(t *testing.T) { + root := t.TempDir() + runner := &fakeRunner{results: []CommandResult{ + {Stdout: root + "\n"}, + }} + + _, err := CreateBranch(context.Background(), BranchOptions{ + Cwd: root, + RunGit: runner.Run, + }) + if err == nil { + t.Fatal("expected error for empty branch name, got nil") + } + }) +} + +func TestIsDefaultBranch(t *testing.T) { + t.Run("ResolvesCurrentBranchAndFallsBackToHeuristic", func(t *testing.T) { + root := t.TempDir() + runner := &fakeRunner{results: []CommandResult{ + {Stdout: root + "\n"}, + {Stdout: "main\n"}, + {}, // ls-remote --symref (no match → heuristic fallback) + }} + + isDefault, branch, err := IsDefaultBranch(context.Background(), DefaultBranchOptions{ + Cwd: root, + RunGit: runner.Run, + }) + if err != nil { + t.Fatalf("IsDefaultBranch returned error: %v", err) + } + if !isDefault || branch != "main" { + t.Fatalf("unexpected result: isDefault=%v branch=%q", isDefault, branch) + } + }) + + t.Run("ExplicitNonDefaultBranch", func(t *testing.T) { + root := t.TempDir() + runner := &fakeRunner{results: []CommandResult{ + {Stdout: root + "\n"}, + {}, // ls-remote --symref (no match → heuristic fallback) + }} + + isDefault, branch, err := IsDefaultBranch(context.Background(), DefaultBranchOptions{ + Cwd: root, + Branch: "feat/some-feature", + RunGit: runner.Run, + }) + if err != nil { + t.Fatalf("IsDefaultBranch returned error: %v", err) + } + if isDefault || branch != "feat/some-feature" { + t.Fatalf("unexpected result: isDefault=%v branch=%q", isDefault, branch) + } + }) +} + +func TestCurrentGitUser(t *testing.T) { + root := t.TempDir() + runner := &fakeRunner{results: []CommandResult{ + {Stdout: "Alex Example\n"}, + }} + + if got := CurrentGitUser(context.Background(), root, runner.Run); got != "Alex Example" { + t.Fatalf("CurrentGitUser = %q, want %q", got, "Alex Example") + } + if got := runner.commandLine(0); got != "git config user.name" { + t.Fatalf("unexpected command: %q", got) + } +} + +func TestSlugifyBranchComponent(t *testing.T) { + cases := map[string]string{ + "Fix Typo In README": "fix-typo-in-readme", + " leading/trailing --": "leading-trailing", + "already-kebab-case": "already-kebab-case", + "": "", + "UPPER_CASE_with--dashes": "upper-case-with-dashes", + } + for input, want := range cases { + if got := SlugifyBranchComponent(input); got != want { + t.Errorf("SlugifyBranchComponent(%q) = %q, want %q", input, got, want) + } + } + + long := strings.Repeat("a", 60) + if got := SlugifyBranchComponent(long); len(got) > maxSlugComponentLen { + t.Fatalf("SlugifyBranchComponent did not cap length: got %d chars", len(got)) + } +} + +func TestBuildBranchName(t *testing.T) { + if got := BuildBranchName("Alice", "Fix Typo"); got != "alice/fix-typo" { + t.Fatalf("BuildBranchName = %q, want %q", got, "alice/fix-typo") + } + if got := BuildBranchName("", ""); got != "user/changes" { + t.Fatalf("BuildBranchName with empty inputs = %q, want %q", got, "user/changes") + } +} From 8fb4f08ecb3d77b9a5ffbd583758836100abf22d Mon Sep 17 00:00:00 2001 From: euxaristia <25621994+euxaristia@users.noreply.github.com> Date: Tue, 14 Jul 2026 11:21:16 -0400 Subject: [PATCH 2/5] fix(zerogit): normalize LLM branch slugs, bound remote lookup, handle existing branches Address CodeRabbit's review on the auto-branch-naming PR: - generateAutoBranchSlug now takes the first non-empty, quote-trimmed line of the model's response before slugifying, instead of folding any preamble or wrapping quotes into the branch name. - isDefaultBranch's ls-remote lookup is now bounded by a 5s timeout so a slow or unreachable remote can't stall push/pr; falls back to the local main/master heuristic as before. - CreateBranch checks whether the target branch already exists locally and checks it out instead of failing checkout -b on the collision. Also add regression coverage: CurrentGitUser's OS-username fallback tier, CreateBranch's existing-branch and generic checkout-failure paths, a CLI-level test for changes pr (previously untested) mirroring the existing changes push coverage, and a messy-LLM-response case for the slug normalization fix. --- internal/cli/workflow_test.go | 79 ++++++++++++++++++++++++++++++++ internal/cli/workflows.go | 18 +++++++- internal/zerogit/zerogit.go | 21 ++++++++- internal/zerogit/zerogit_test.go | 79 +++++++++++++++++++++++++++++++- 4 files changed, 194 insertions(+), 3 deletions(-) diff --git a/internal/cli/workflow_test.go b/internal/cli/workflow_test.go index 8be4a1b54..c49f1435e 100644 --- a/internal/cli/workflow_test.go +++ b/internal/cli/workflow_test.go @@ -864,6 +864,42 @@ func TestEnsureFeatureBranchUsesLLMSlugWhenProviderConfigured(t *testing.T) { } } +func TestEnsureFeatureBranchNormalizesMessyLLMSlugResponse(t *testing.T) { + cwd := t.TempDir() + // The prompt asks the model for "ONLY the raw slug text," but models don't + // always comply: this response wraps the actual slug in quotes with a + // blank line first. Slugifying the whole response verbatim would fold that + // noise into the branch name instead of just "add-login-page". + mockProv := &mockCommitMsgProvider{response: "\n\"add login page\"\n"} + var createdName string + + branch, err := ensureFeatureBranch(context.Background(), &bytes.Buffer{}, false, cwd, false, false, appDeps{ + isDefaultBranch: func(ctx context.Context, options zerogit.DefaultBranchOptions) (bool, string, error) { + return true, "main", nil + }, + inspectChanges: func(ctx context.Context, options zerogit.InspectOptions) (zerogit.ChangeSummary, error) { + return zerogit.ChangeSummary{Files: []zerogit.FileChange{{Path: "login.go", Status: "added"}}, Diff: "+func Login() {}"}, nil + }, + resolveConfig: func(workspaceRoot string, overrides config.Overrides) (config.ResolvedConfig, error) { + return execResolvedConfig(), nil + }, + newProvider: func(profile config.ProviderProfile) (zeroruntime.Provider, error) { + return mockProv, nil + }, + currentGitUser: func(ctx context.Context, cwd string) string { return "Someone" }, + createBranch: func(ctx context.Context, options zerogit.BranchOptions) (zerogit.BranchResult, error) { + createdName = options.Name + return zerogit.BranchResult{Branch: options.Name}, nil + }, + }) + if err != nil { + t.Fatalf("ensureFeatureBranch returned error: %v", err) + } + if branch != "someone/add-login-page" || createdName != "someone/add-login-page" { + t.Fatalf("unexpected branch: got %q (created %q)", branch, createdName) + } +} + func TestEnsureFeatureBranchSkipsWhenNotOnDefault(t *testing.T) { cwd := t.TempDir() createBranchCalled := false @@ -952,6 +988,49 @@ func TestRunChangesPushCreatesFeatureBranchWhenOnDefault(t *testing.T) { } } +func TestRunChangesPRCreatesFeatureBranchWhenOnDefault(t *testing.T) { + cwd := t.TempDir() + var pushedBranch string + + var stdout, stderr bytes.Buffer + exitCode := runWithDeps([]string{"changes", "pr", "--fill"}, &stdout, &stderr, appDeps{ + getwd: func() (string, error) { return cwd, nil }, + isDefaultBranch: func(ctx context.Context, options zerogit.DefaultBranchOptions) (bool, string, error) { + return true, "main", nil + }, + inspectChanges: func(ctx context.Context, options zerogit.InspectOptions) (zerogit.ChangeSummary, error) { + return zerogit.ChangeSummary{Files: []zerogit.FileChange{{Path: "README.md", Status: "modified"}}}, nil + }, + resolveConfig: func(workspaceRoot string, overrides config.Overrides) (config.ResolvedConfig, error) { + return config.ResolvedConfig{}, nil + }, + currentGitUser: func(ctx context.Context, cwd string) string { return "Someone" }, + createBranch: func(ctx context.Context, options zerogit.BranchOptions) (zerogit.BranchResult, error) { + return zerogit.BranchResult{Branch: options.Name}, nil + }, + pushChanges: func(ctx context.Context, options zerogit.PushOptions) (zerogit.PushResult, error) { + pushedBranch = options.Branch + return zerogit.PushResult{Remote: "origin", Branch: options.Branch}, nil + }, + createPR: func(ctx context.Context, options zerogit.PROptions) (zerogit.PRResult, error) { + return zerogit.PRResult{Output: "https://example.invalid/pr/1"}, nil + }, + }) + + if exitCode != exitSuccess { + t.Fatalf("expected exit code %d, got %d: %s", exitSuccess, exitCode, stderr.String()) + } + // runChangesPR hardcodes dryRun=false into ensureFeatureBranch (unlike push, + // which forwards options.dryRun), so it always creates and forwards the + // branch on the default branch, with no --dry-run bypass to verify here. + if pushedBranch != "someone/readme-md" { + t.Fatalf("expected pushChanges to target the newly created branch, got %q", pushedBranch) + } + if !strings.Contains(stdout.String(), "Created branch someone/readme-md") { + t.Fatalf("expected branch-creation message in stdout, got %q", stdout.String()) + } +} + func TestRunChangesPushSkipsBranchCreationWithYes(t *testing.T) { cwd := t.TempDir() isDefaultBranchCalled := false diff --git a/internal/cli/workflows.go b/internal/cli/workflows.go index 613d6546a..d4a553cbc 100644 --- a/internal/cli/workflows.go +++ b/internal/cli/workflows.go @@ -1108,9 +1108,25 @@ func generateAutoBranchSlug(ctx context.Context, provider zeroruntime.Provider, return "", fmt.Errorf("%s", collected.Error) } - slug := zerogit.SlugifyBranchComponent(collected.Text) + slug := zerogit.SlugifyBranchComponent(firstNonEmptyBranchSlugLine(collected.Text)) if slug == "" { return "", fmt.Errorf("provider returned empty branch slug") } return slug, nil } + +// firstNonEmptyBranchSlugLine picks the actual slug out of a model response +// that didn't follow the "output only the raw slug" instruction exactly — +// preamble/trailing blank lines or a model wrapping its answer in quotes. +// SlugifyBranchComponent alone would fold that whole response, quotes and +// all, into one long slug instead of just the intended words. +func firstNonEmptyBranchSlugLine(text string) string { + for _, line := range strings.Split(text, "\n") { + line = strings.Trim(strings.TrimSpace(line), `"'`) + line = strings.TrimSpace(line) + if line != "" { + return line + } + } + return "" +} diff --git a/internal/zerogit/zerogit.go b/internal/zerogit/zerogit.go index b49ab4790..7ff4d3294 100644 --- a/internal/zerogit/zerogit.go +++ b/internal/zerogit/zerogit.go @@ -10,6 +10,7 @@ import ( "path/filepath" "regexp" "strings" + "time" "unicode/utf8" "github.com/Gitlawb/zero/internal/redaction" @@ -606,8 +607,16 @@ func Push(ctx context.Context, options PushOptions) (PushResult, error) { }, nil } +// isDefaultBranchRemoteLookupTimeout bounds the ls-remote HEAD-symref check +// below, so a caller passing context.Background() (as ensureFeatureBranch +// does) can't stall push/pr indefinitely on a slow or unreachable remote; the +// local main/master fallback below covers the timeout case. +const isDefaultBranchRemoteLookupTimeout = 5 * time.Second + func isDefaultBranch(ctx context.Context, runGit Runner, dir, remote, branch string) bool { - if out, err := gitOutput(ctx, runGit, dir, "ls-remote", "--symref", remote, "HEAD"); err == nil { + lookupCtx, cancel := context.WithTimeout(ctx, isDefaultBranchRemoteLookupTimeout) + defer cancel() + if out, err := gitOutput(lookupCtx, runGit, dir, "ls-remote", "--symref", remote, "HEAD"); err == nil { for _, line := range strings.Split(out, "\n") { line = strings.TrimSpace(line) if strings.HasPrefix(line, "ref: refs/heads/") && strings.HasSuffix(line, "\tHEAD") { @@ -697,6 +706,16 @@ func CreateBranch(ctx context.Context, options BranchOptions) (BranchResult, err if options.DryRun { return BranchResult{Branch: name}, nil } + // A repeated run (e.g. the same diff producing the same slug, or a retry + // after a prior push under this name) can target a branch that already + // exists locally. `checkout -b` would fail on that collision, so check + // first and check it out directly instead of erroring. + if _, err := gitOutput(ctx, runGit, root, "rev-parse", "--verify", "--quiet", "refs/heads/"+name); err == nil { + if _, err := gitOutput(ctx, runGit, root, "checkout", name); err != nil { + return BranchResult{}, fmt.Errorf("checkout existing branch %q: %w", name, err) + } + return BranchResult{Branch: name}, nil + } if _, err := gitOutput(ctx, runGit, root, "checkout", "-b", name); err != nil { return BranchResult{}, fmt.Errorf("create branch %q: %w", name, err) } diff --git a/internal/zerogit/zerogit_test.go b/internal/zerogit/zerogit_test.go index 853e665c6..56b0b085f 100644 --- a/internal/zerogit/zerogit_test.go +++ b/internal/zerogit/zerogit_test.go @@ -4,6 +4,7 @@ import ( "context" "os" "os/exec" + "os/user" "path/filepath" "reflect" "strings" @@ -733,6 +734,7 @@ func TestCreateBranch(t *testing.T) { root := t.TempDir() runner := &fakeRunner{results: []CommandResult{ {Stdout: root + "\n"}, + {ExitCode: 1}, // rev-parse --verify: no local branch by that name yet {Stdout: "Switched to a new branch 'alice/fix-typo'\n"}, }} @@ -747,11 +749,56 @@ func TestCreateBranch(t *testing.T) { if result.Branch != "alice/fix-typo" { t.Fatalf("unexpected branch: %#v", result) } - if got := runner.commandLine(1); got != "git checkout -b alice/fix-typo" { + if got := runner.commandLine(1); got != "git rev-parse --verify --quiet refs/heads/alice/fix-typo" { + t.Fatalf("unexpected existence-check command: %q", got) + } + if got := runner.commandLine(2); got != "git checkout -b alice/fix-typo" { t.Fatalf("unexpected checkout command: %q", got) } }) + t.Run("ChecksOutExistingBranchInsteadOfFailing", func(t *testing.T) { + root := t.TempDir() + runner := &fakeRunner{results: []CommandResult{ + {Stdout: root + "\n"}, + {Stdout: "abc1234\n"}, // rev-parse --verify: branch already exists locally + {Stdout: "Switched to branch 'alice/fix-typo'\n"}, + }} + + result, err := CreateBranch(context.Background(), BranchOptions{ + Cwd: root, + Name: "alice/fix-typo", + RunGit: runner.Run, + }) + if err != nil { + t.Fatalf("CreateBranch returned error: %v", err) + } + if result.Branch != "alice/fix-typo" { + t.Fatalf("unexpected branch: %#v", result) + } + if got := runner.commandLine(2); got != "git checkout alice/fix-typo" { + t.Fatalf("expected a plain checkout of the existing branch, got %q", got) + } + }) + + t.Run("PropagatesCheckoutFailureForNewBranch", func(t *testing.T) { + root := t.TempDir() + runner := &fakeRunner{results: []CommandResult{ + {Stdout: root + "\n"}, + {ExitCode: 1}, // rev-parse --verify: no local branch by that name yet + {ExitCode: 128, Stderr: "fatal: unable to write new index file"}, + }} + + _, err := CreateBranch(context.Background(), BranchOptions{ + Cwd: root, + Name: "alice/fix-typo", + RunGit: runner.Run, + }) + if err == nil || !strings.Contains(err.Error(), "unable to write new index file") { + t.Fatalf("expected wrapped checkout failure, got %v", err) + } + }) + t.Run("DryRunDoesNotCheckout", func(t *testing.T) { root := t.TempDir() runner := &fakeRunner{results: []CommandResult{ @@ -847,6 +894,36 @@ func TestCurrentGitUser(t *testing.T) { } } +// TestCurrentGitUserFallsBackToOSUsername covers the second of CurrentGitUser's +// three fallback tiers: when `git config user.name` fails or returns nothing, +// it falls back to the OS account username. The third tier (the literal +// "user") only triggers when os/user.Current itself fails, which isn't +// practical to force here without adding an injectable seam for it solely for +// this coverage gap. +func TestCurrentGitUserFallsBackToOSUsername(t *testing.T) { + root := t.TempDir() + want, err := user.Current() + if err != nil || want.Username == "" { + t.Skip("no OS user available to compare against in this environment") + } + + cases := map[string][]CommandResult{ + "ConfigCommandErrors": {{ExitCode: 1, Stderr: "fatal: unable to read config"}}, + "ConfigCommandEmpty": {{Stdout: ""}}, + } + for name, results := range cases { + t.Run(name, func(t *testing.T) { + runner := &fakeRunner{results: results} + if got := CurrentGitUser(context.Background(), root, runner.Run); got != want.Username { + t.Fatalf("CurrentGitUser = %q, want OS username %q", got, want.Username) + } + if got := runner.commandLine(0); got != "git config user.name" { + t.Fatalf("unexpected command: %q", got) + } + }) + } +} + func TestSlugifyBranchComponent(t *testing.T) { cases := map[string]string{ "Fix Typo In README": "fix-typo-in-readme", From e3ec76f3d9d6bfb75ef6f6c1fbe9052db8fcfc69 Mon Sep 17 00:00:00 2001 From: euxaristia <25621994+euxaristia@users.noreply.github.com> Date: Wed, 15 Jul 2026 09:07:35 -0400 Subject: [PATCH 3/5] fix(zerogit,cli): make auto-branching collision-safe, remote-aware, fail-closed, and opt-in for LLM naming - CreateBranch never checks out an existing branch on a name collision: it picks a unique suffixed name off the current HEAD (name-2 .. name-9) and fails visibly when the namespace is exhausted, so a stale branch with unrelated history can no longer be published while the new commit stays behind on the default branch. - ensureFeatureBranch resolves the remote once (explicit --remote, then the original branch's configured upstream, then origin) via IsDefaultBranch, which now reports the resolved remote, and push/pr thread that remote into Push, so a freshly created branch without tracking configuration no longer falls back to origin in fork setups. - The default-branch check fails closed: main and master count as default with no network, an unreachable remote falls back to the local refs/remotes//HEAD record, and when neither answers the push is refused with guidance instead of silently dropping the confirmation guard for repositories whose default is trunk or develop. - LLM branch naming is now opt-in via --auto on push/pr, matching commit --auto: a configured provider alone no longer causes the change diff to be uploaded during ordinary git-only pushes. - After the ordinary commit-then-push sequence the tree is clean, so the fallback name comes from the HEAD commit subject instead of the constant user/changes. --- internal/cli/app.go | 9 +- internal/cli/workflow_test.go | 147 ++++++++++++++++++++++++++----- internal/cli/workflows.go | 84 ++++++++++++------ internal/zerogit/zerogit.go | 95 +++++++++++++++----- internal/zerogit/zerogit_test.go | 117 ++++++++++++++++++++---- 5 files changed, 362 insertions(+), 90 deletions(-) diff --git a/internal/cli/app.go b/internal/cli/app.go index 2b0b7f28f..035d42db0 100644 --- a/internal/cli/app.go +++ b/internal/cli/app.go @@ -84,8 +84,9 @@ type appDeps struct { pushChanges func(context.Context, zerogit.PushOptions) (zerogit.PushResult, error) createPR func(context.Context, zerogit.PROptions) (zerogit.PRResult, error) createBranch func(context.Context, zerogit.BranchOptions) (zerogit.BranchResult, error) - isDefaultBranch func(context.Context, zerogit.DefaultBranchOptions) (bool, string, error) + isDefaultBranch func(context.Context, zerogit.DefaultBranchOptions) (bool, string, string, error) currentGitUser func(context.Context, string) string + headCommitSubject func(context.Context, string) string runTUI func(context.Context, tui.Options) int runEditor func(string) error checkUpdate func(context.Context, update.Options) (update.Result, error) @@ -196,6 +197,9 @@ func defaultAppDeps() appDeps { currentGitUser: func(ctx context.Context, cwd string) string { return zerogit.CurrentGitUser(ctx, cwd, nil) }, + headCommitSubject: func(ctx context.Context, cwd string) string { + return zerogit.HeadCommitSubject(ctx, cwd, nil) + }, runTUI: tui.Run, runEditor: openEditor, checkUpdate: update.Check, @@ -554,6 +558,9 @@ func fillAppDeps(deps appDeps) appDeps { if deps.currentGitUser == nil { deps.currentGitUser = defaults.currentGitUser } + if deps.headCommitSubject == nil { + deps.headCommitSubject = defaults.headCommitSubject + } if deps.runTUI == nil { deps.runTUI = defaults.runTUI } diff --git a/internal/cli/workflow_test.go b/internal/cli/workflow_test.go index c49f1435e..03ff23c70 100644 --- a/internal/cli/workflow_test.go +++ b/internal/cli/workflow_test.go @@ -808,9 +808,9 @@ func TestEnsureFeatureBranchCreatesBranchOffDefaultWithoutProvider(t *testing.T) cwd := t.TempDir() var createdName string - branch, err := ensureFeatureBranch(context.Background(), &bytes.Buffer{}, false, cwd, false, false, appDeps{ - isDefaultBranch: func(ctx context.Context, options zerogit.DefaultBranchOptions) (bool, string, error) { - return true, "main", nil + branch, _, err := ensureFeatureBranch(context.Background(), &bytes.Buffer{}, false, cwd, "", false, false, false, appDeps{ + isDefaultBranch: func(ctx context.Context, options zerogit.DefaultBranchOptions) (bool, string, string, error) { + return true, "main", "origin", nil }, inspectChanges: func(ctx context.Context, options zerogit.InspectOptions) (zerogit.ChangeSummary, error) { return zerogit.ChangeSummary{Files: []zerogit.FileChange{{Path: "README.md", Status: "modified"}}}, nil @@ -837,9 +837,9 @@ func TestEnsureFeatureBranchUsesLLMSlugWhenProviderConfigured(t *testing.T) { mockProv := &mockCommitMsgProvider{response: "add login page"} var createdName string - branch, err := ensureFeatureBranch(context.Background(), &bytes.Buffer{}, false, cwd, false, false, appDeps{ - isDefaultBranch: func(ctx context.Context, options zerogit.DefaultBranchOptions) (bool, string, error) { - return true, "main", nil + branch, _, err := ensureFeatureBranch(context.Background(), &bytes.Buffer{}, false, cwd, "", false, false, true, appDeps{ + isDefaultBranch: func(ctx context.Context, options zerogit.DefaultBranchOptions) (bool, string, string, error) { + return true, "main", "origin", nil }, inspectChanges: func(ctx context.Context, options zerogit.InspectOptions) (zerogit.ChangeSummary, error) { return zerogit.ChangeSummary{Files: []zerogit.FileChange{{Path: "login.go", Status: "added"}}, Diff: "+func Login() {}"}, nil @@ -873,9 +873,9 @@ func TestEnsureFeatureBranchNormalizesMessyLLMSlugResponse(t *testing.T) { mockProv := &mockCommitMsgProvider{response: "\n\"add login page\"\n"} var createdName string - branch, err := ensureFeatureBranch(context.Background(), &bytes.Buffer{}, false, cwd, false, false, appDeps{ - isDefaultBranch: func(ctx context.Context, options zerogit.DefaultBranchOptions) (bool, string, error) { - return true, "main", nil + branch, _, err := ensureFeatureBranch(context.Background(), &bytes.Buffer{}, false, cwd, "", false, false, true, appDeps{ + isDefaultBranch: func(ctx context.Context, options zerogit.DefaultBranchOptions) (bool, string, string, error) { + return true, "main", "origin", nil }, inspectChanges: func(ctx context.Context, options zerogit.InspectOptions) (zerogit.ChangeSummary, error) { return zerogit.ChangeSummary{Files: []zerogit.FileChange{{Path: "login.go", Status: "added"}}, Diff: "+func Login() {}"}, nil @@ -904,9 +904,9 @@ func TestEnsureFeatureBranchSkipsWhenNotOnDefault(t *testing.T) { cwd := t.TempDir() createBranchCalled := false - branch, err := ensureFeatureBranch(context.Background(), &bytes.Buffer{}, false, cwd, false, false, appDeps{ - isDefaultBranch: func(ctx context.Context, options zerogit.DefaultBranchOptions) (bool, string, error) { - return false, "feat/existing", nil + branch, _, err := ensureFeatureBranch(context.Background(), &bytes.Buffer{}, false, cwd, "", false, false, false, appDeps{ + isDefaultBranch: func(ctx context.Context, options zerogit.DefaultBranchOptions) (bool, string, string, error) { + return false, "feat/existing", "origin", nil }, createBranch: func(ctx context.Context, options zerogit.BranchOptions) (zerogit.BranchResult, error) { createBranchCalled = true @@ -935,10 +935,10 @@ func TestEnsureFeatureBranchSkipsWhenAllowDefaultOrDryRun(t *testing.T) { } { t.Run(tc.name, func(t *testing.T) { cwd := t.TempDir() - branch, err := ensureFeatureBranch(context.Background(), &bytes.Buffer{}, false, cwd, tc.allowDefaultBranch, tc.dryRun, appDeps{ - isDefaultBranch: func(ctx context.Context, options zerogit.DefaultBranchOptions) (bool, string, error) { + branch, _, err := ensureFeatureBranch(context.Background(), &bytes.Buffer{}, false, cwd, "", tc.allowDefaultBranch, tc.dryRun, false, appDeps{ + isDefaultBranch: func(ctx context.Context, options zerogit.DefaultBranchOptions) (bool, string, string, error) { t.Fatal("isDefaultBranch should not be called") - return false, "", nil + return false, "", "", nil }, }) if err != nil { @@ -951,6 +951,111 @@ func TestEnsureFeatureBranchSkipsWhenAllowDefaultOrDryRun(t *testing.T) { } } +func TestEnsureFeatureBranchNamesFromHeadCommitAfterCommit(t *testing.T) { + // The ordinary sequence is `changes commit` then `changes push`: the + // working tree is clean by the time the branch is named, so the + // diff-derived fallback would always be the meaningless "changes". The + // name must come from the commit being pushed instead. + cwd := t.TempDir() + var createdName string + + branch, _, err := ensureFeatureBranch(context.Background(), &bytes.Buffer{}, false, cwd, "", false, false, false, appDeps{ + isDefaultBranch: func(ctx context.Context, options zerogit.DefaultBranchOptions) (bool, string, string, error) { + return true, "main", "origin", nil + }, + inspectChanges: func(ctx context.Context, options zerogit.InspectOptions) (zerogit.ChangeSummary, error) { + return zerogit.ChangeSummary{}, nil // clean tree: commit already made + }, + headCommitSubject: func(ctx context.Context, cwd string) string { + return "fix(parser): handle empty input" + }, + resolveConfig: func(workspaceRoot string, overrides config.Overrides) (config.ResolvedConfig, error) { + return config.ResolvedConfig{}, nil + }, + currentGitUser: func(ctx context.Context, cwd string) string { return "Someone" }, + createBranch: func(ctx context.Context, options zerogit.BranchOptions) (zerogit.BranchResult, error) { + createdName = options.Name + return zerogit.BranchResult{Branch: options.Name}, nil + }, + }) + if err != nil { + t.Fatalf("ensureFeatureBranch returned error: %v", err) + } + if branch != createdName || !strings.HasPrefix(branch, "someone/fix-parser") { + t.Fatalf("expected a name derived from the HEAD commit subject, got %q", branch) + } +} + +func TestEnsureFeatureBranchDoesNotCallProviderWithoutAuto(t *testing.T) { + // changes push/pr were git-only commands: a configured provider must not + // cause the change diff to be uploaded for naming unless --auto opts in. + cwd := t.TempDir() + + branch, _, err := ensureFeatureBranch(context.Background(), &bytes.Buffer{}, false, cwd, "", false, false, false, appDeps{ + isDefaultBranch: func(ctx context.Context, options zerogit.DefaultBranchOptions) (bool, string, string, error) { + return true, "main", "origin", nil + }, + inspectChanges: func(ctx context.Context, options zerogit.InspectOptions) (zerogit.ChangeSummary, error) { + return zerogit.ChangeSummary{Files: []zerogit.FileChange{{Path: "login.go", Status: "added"}}, Diff: "+func Login() {}"}, nil + }, + resolveConfig: func(workspaceRoot string, overrides config.Overrides) (config.ResolvedConfig, error) { + return execResolvedConfig(), nil // provider IS configured + }, + newProvider: func(profile config.ProviderProfile) (zeroruntime.Provider, error) { + t.Fatal("provider must not be constructed without --auto") + return nil, nil + }, + currentGitUser: func(ctx context.Context, cwd string) string { return "Someone" }, + createBranch: func(ctx context.Context, options zerogit.BranchOptions) (zerogit.BranchResult, error) { + return zerogit.BranchResult{Branch: options.Name}, nil + }, + }) + if err != nil { + t.Fatalf("ensureFeatureBranch returned error: %v", err) + } + if branch != "someone/login-go" { + t.Fatalf("expected the deterministic local name, got %q", branch) + } +} + +func TestRunChangesPushUsesResolvedRemoteForNewBranch(t *testing.T) { + // In a fork setup the original branch tracks a non-origin remote. The + // freshly created feature branch has no tracking configuration, so the + // resolved remote must be threaded into Push explicitly or it would + // silently fall back to origin. + cwd := t.TempDir() + var pushedRemote string + + var stdout, stderr bytes.Buffer + exitCode := runWithDeps([]string{"changes", "push"}, &stdout, &stderr, appDeps{ + getwd: func() (string, error) { return cwd, nil }, + isDefaultBranch: func(ctx context.Context, options zerogit.DefaultBranchOptions) (bool, string, string, error) { + return true, "main", "upstream", nil + }, + inspectChanges: func(ctx context.Context, options zerogit.InspectOptions) (zerogit.ChangeSummary, error) { + return zerogit.ChangeSummary{Files: []zerogit.FileChange{{Path: "README.md", Status: "modified"}}}, nil + }, + resolveConfig: func(workspaceRoot string, overrides config.Overrides) (config.ResolvedConfig, error) { + return config.ResolvedConfig{}, nil + }, + currentGitUser: func(ctx context.Context, cwd string) string { return "Someone" }, + createBranch: func(ctx context.Context, options zerogit.BranchOptions) (zerogit.BranchResult, error) { + return zerogit.BranchResult{Branch: options.Name}, nil + }, + pushChanges: func(ctx context.Context, options zerogit.PushOptions) (zerogit.PushResult, error) { + pushedRemote = options.Remote + return zerogit.PushResult{Remote: options.Remote, Branch: options.Branch}, nil + }, + }) + + if exitCode != exitSuccess { + t.Fatalf("expected exit code %d, got %d: %s", exitSuccess, exitCode, stderr.String()) + } + if pushedRemote != "upstream" { + t.Fatalf("expected push to target the resolved remote %q, got %q", "upstream", pushedRemote) + } +} + func TestRunChangesPushCreatesFeatureBranchWhenOnDefault(t *testing.T) { cwd := t.TempDir() var pushedBranch string @@ -958,8 +1063,8 @@ func TestRunChangesPushCreatesFeatureBranchWhenOnDefault(t *testing.T) { var stdout, stderr bytes.Buffer exitCode := runWithDeps([]string{"changes", "push"}, &stdout, &stderr, appDeps{ getwd: func() (string, error) { return cwd, nil }, - isDefaultBranch: func(ctx context.Context, options zerogit.DefaultBranchOptions) (bool, string, error) { - return true, "main", nil + isDefaultBranch: func(ctx context.Context, options zerogit.DefaultBranchOptions) (bool, string, string, error) { + return true, "main", "origin", nil }, inspectChanges: func(ctx context.Context, options zerogit.InspectOptions) (zerogit.ChangeSummary, error) { return zerogit.ChangeSummary{Files: []zerogit.FileChange{{Path: "README.md", Status: "modified"}}}, nil @@ -995,8 +1100,8 @@ func TestRunChangesPRCreatesFeatureBranchWhenOnDefault(t *testing.T) { var stdout, stderr bytes.Buffer exitCode := runWithDeps([]string{"changes", "pr", "--fill"}, &stdout, &stderr, appDeps{ getwd: func() (string, error) { return cwd, nil }, - isDefaultBranch: func(ctx context.Context, options zerogit.DefaultBranchOptions) (bool, string, error) { - return true, "main", nil + isDefaultBranch: func(ctx context.Context, options zerogit.DefaultBranchOptions) (bool, string, string, error) { + return true, "main", "origin", nil }, inspectChanges: func(ctx context.Context, options zerogit.InspectOptions) (zerogit.ChangeSummary, error) { return zerogit.ChangeSummary{Files: []zerogit.FileChange{{Path: "README.md", Status: "modified"}}}, nil @@ -1038,9 +1143,9 @@ func TestRunChangesPushSkipsBranchCreationWithYes(t *testing.T) { var stdout, stderr bytes.Buffer exitCode := runWithDeps([]string{"changes", "push", "--yes"}, &stdout, &stderr, appDeps{ getwd: func() (string, error) { return cwd, nil }, - isDefaultBranch: func(ctx context.Context, options zerogit.DefaultBranchOptions) (bool, string, error) { + isDefaultBranch: func(ctx context.Context, options zerogit.DefaultBranchOptions) (bool, string, string, error) { isDefaultBranchCalled = true - return true, "main", nil + return true, "main", "origin", nil }, pushChanges: func(ctx context.Context, options zerogit.PushOptions) (zerogit.PushResult, error) { if options.Branch != "" { diff --git a/internal/cli/workflows.go b/internal/cli/workflows.go index d4a553cbc..38f85608f 100644 --- a/internal/cli/workflows.go +++ b/internal/cli/workflows.go @@ -523,8 +523,8 @@ func parseChangesArgs(args []string, command string) (changesCommandOptions, boo if command != "commit" && options.message != "" { return options, false, execUsageError{"--message is only valid with `zero changes commit`"} } - if command != "commit" && (options.hasMessage || options.dryRun || options.auto) { - return options, false, execUsageError{"--message, --dry-run, and --auto are only valid with `zero changes commit`"} + if command != "commit" && options.hasMessage { + return options, false, execUsageError{"--message is only valid with `zero changes commit`"} } if command == "commit" && options.hasMessage && options.auto { return options, false, execUsageError{"cannot specify both --message and --auto"} @@ -532,6 +532,11 @@ func parseChangesArgs(args []string, command string) (changesCommandOptions, boo if command != "commit" && command != "push" && options.dryRun { return options, false, execUsageError{"--dry-run is only valid with commit or push"} } + // --auto on push/pr is the explicit opt-in for LLM branch naming (see + // ensureFeatureBranch); on commit it opts into the LLM commit message. + if command != "commit" && command != "push" && command != "pr" && options.auto { + return options, false, execUsageError{"--auto is only valid with commit, push, or pr"} + } if command != "inspect" && options.baseRef != "" { return options, false, execUsageError{"--base is only valid with `zero changes inspect`"} } @@ -839,8 +844,7 @@ Flags: --fill Automatically populate PR title and body from commits --draft Create PR as a draft --yes Confirm pushing to a default/protected branch - -a, --auto Auto-generate commit message using LLM (use --dry-run to preview) - --dry-run Preview commit metadata without mutating git state + -a, --auto Use the LLM: commit generates the message, push/pr name the auto-created branch (sends the diff to the provider) --json Print JSON output -h, --help Show this help `) @@ -863,14 +867,14 @@ func runChangesPush(args []string, stdout io.Writer, stderr io.Writer, deps appD return writeExecUsageError(stderr, err.Error()) } - branch, err := ensureFeatureBranch(context.Background(), stdout, options.json, workspaceRoot, options.yes, options.dryRun, deps) + branch, remote, err := ensureFeatureBranch(context.Background(), stdout, options.json, workspaceRoot, options.remote, options.yes, options.dryRun, options.auto, deps) if err != nil { return writeExecUsageError(stderr, err.Error()) } result, err := deps.pushChanges(context.Background(), zerogit.PushOptions{ Cwd: workspaceRoot, - Remote: options.remote, + Remote: firstNonEmptyString(options.remote, remote), Branch: branch, Force: options.force, DryRun: options.dryRun, @@ -921,7 +925,7 @@ func runChangesPR(args []string, stdout io.Writer, stderr io.Writer, deps appDep return writeExecUsageError(stderr, err.Error()) } - branch, err := ensureFeatureBranch(context.Background(), stdout, options.json, workspaceRoot, options.yes, false, deps) + branch, remote, err := ensureFeatureBranch(context.Background(), stdout, options.json, workspaceRoot, options.remote, options.yes, false, options.auto, deps) if err != nil { return writeExecUsageError(stderr, err.Error()) } @@ -933,7 +937,7 @@ func runChangesPR(args []string, stdout io.Writer, stderr io.Writer, deps appDep } pushResult, err := deps.pushChanges(context.Background(), zerogit.PushOptions{ Cwd: workspaceRoot, - Remote: options.remote, + Remote: firstNonEmptyString(options.remote, remote), Branch: branch, Force: options.force, AllowPushDefaultBranch: options.yes, @@ -1023,38 +1027,60 @@ func generateAutoCommitMessage(ctx context.Context, provider zeroruntime.Provide // zerogit.Push, so rather than surface that as a dead end, create and switch // to a conventionally named "/" branch first. It returns the // branch push/pr should target, or "" to mean "current HEAD branch, unchanged" -// (zerogit.Push already treats an empty Branch that way). allowDefaultBranch -// (the --yes flag) and dryRun both opt out via that "" return, leaving Push's -// own guard/preview behavior on the default branch unaffected. -func ensureFeatureBranch(ctx context.Context, stdout io.Writer, jsonMode bool, workspaceRoot string, allowDefaultBranch bool, dryRun bool, deps appDeps) (string, error) { +// (zerogit.Push already treats an empty Branch that way), plus the remote the +// preflight resolved (requestedRemote, then the original branch's configured +// upstream, then "origin"). Callers must pass that remote to Push: a freshly +// created branch has no tracking configuration, so Push's own fallback would +// silently retarget "origin" even when the work came from a branch tracking +// a different remote. allowDefaultBranch (the --yes flag) and dryRun both opt +// out via the "" return, leaving Push's own guard/preview behavior on the +// default branch unaffected. +// +// autoNaming gates the LLM naming path (--auto): these commands were +// git-only, and sending the change diff to a configured provider on every +// default-branch push would silently export source code nobody asked to +// share. Without the opt-in the name comes from deterministic local +// information only. +func ensureFeatureBranch(ctx context.Context, stdout io.Writer, jsonMode bool, workspaceRoot string, requestedRemote string, allowDefaultBranch bool, dryRun bool, autoNaming bool, deps appDeps) (string, string, error) { if allowDefaultBranch || dryRun { - return "", nil + return "", strings.TrimSpace(requestedRemote), nil } - isDefault, currentBranch, err := deps.isDefaultBranch(ctx, zerogit.DefaultBranchOptions{Cwd: workspaceRoot}) + isDefault, currentBranch, remote, err := deps.isDefaultBranch(ctx, zerogit.DefaultBranchOptions{Cwd: workspaceRoot, Remote: requestedRemote}) if err != nil { - return "", err + return "", "", err } if !isDefault { - return currentBranch, nil + return currentBranch, remote, nil } summary, err := deps.inspectChanges(ctx, zerogit.InspectOptions{Cwd: workspaceRoot}) if err != nil { - return "", fmt.Errorf("failed to inspect changes: %w", err) + return "", "", fmt.Errorf("failed to inspect changes: %w", err) } slug := fallbackBranchSlug(summary) - if resolved, cfgErr := deps.resolveConfig(workspaceRoot, config.Overrides{}); cfgErr == nil && config.HasProviderProfile(resolved.Provider) { - if provider, provErr := deps.newProvider(resolved.Provider); provErr == nil { - if !jsonMode { - fmt.Fprintln(stdout, "Generating branch name using LLM...") - } - genCtx, cancel := context.WithTimeout(ctx, 60*time.Second) - generated, genErr := generateAutoBranchSlug(genCtx, provider, resolved.Provider.Model, redactChangeSummary(summary)) - cancel() - if genErr == nil && generated != "" { - slug = generated + if len(summary.Files) == 0 { + // The ordinary sequence is `changes commit` then `changes push`, so + // the working tree here is clean and the diff-derived fallback would + // always be the meaningless "changes". Name the branch from the + // commit that is about to be pushed instead. + if subject := deps.headCommitSubject(ctx, workspaceRoot); subject != "" { + slug = zerogit.SlugifyBranchComponent(subject) + } + } + if autoNaming && strings.TrimSpace(summary.Diff) != "" { + if resolved, cfgErr := deps.resolveConfig(workspaceRoot, config.Overrides{}); cfgErr == nil && config.HasProviderProfile(resolved.Provider) { + if provider, provErr := deps.newProvider(resolved.Provider); provErr == nil { + if !jsonMode { + fmt.Fprintln(stdout, "Generating branch name using LLM...") + } + genCtx, cancel := context.WithTimeout(ctx, 60*time.Second) + generated, genErr := generateAutoBranchSlug(genCtx, provider, resolved.Provider.Model, redactChangeSummary(summary)) + cancel() + if genErr == nil && generated != "" { + slug = generated + } } } } @@ -1062,12 +1088,12 @@ func ensureFeatureBranch(ctx context.Context, stdout io.Writer, jsonMode bool, w name := zerogit.BuildBranchName(deps.currentGitUser(ctx, workspaceRoot), slug) result, err := deps.createBranch(ctx, zerogit.BranchOptions{Cwd: workspaceRoot, Name: name}) if err != nil { - return "", fmt.Errorf("failed to create branch: %w", err) + return "", "", fmt.Errorf("failed to create branch: %w", err) } if !jsonMode { fmt.Fprintf(stdout, "Created branch %s (was on %s)\n", result.Branch, currentBranch) } - return result.Branch, nil + return result.Branch, remote, nil } // fallbackBranchSlug derives a deterministic branch-name slug from a change diff --git a/internal/zerogit/zerogit.go b/internal/zerogit/zerogit.go index 7ff4d3294..b5b8e4085 100644 --- a/internal/zerogit/zerogit.go +++ b/internal/zerogit/zerogit.go @@ -581,7 +581,11 @@ func Push(ctx context.Context, options PushOptions) (PushResult, error) { } if !options.AllowPushDefaultBranch { - if isDefaultBranch(ctx, runGit, root, remote, branch) { + isDefault, err := isDefaultBranch(ctx, runGit, root, remote, branch) + if err != nil { + return PushResult{}, fmt.Errorf("cannot verify %q is not the default/protected branch: %w; use --yes to override", branch, err) + } + if isDefault { return PushResult{}, fmt.Errorf("refusing to push to %q (default/protected branch); use --yes to override", branch) } } @@ -613,7 +617,13 @@ func Push(ctx context.Context, options PushOptions) (PushResult, error) { // local main/master fallback below covers the timeout case. const isDefaultBranchRemoteLookupTimeout = 5 * time.Second -func isDefaultBranch(ctx context.Context, runGit Runner, dir, remote, branch string) bool { +func isDefaultBranch(ctx context.Context, runGit Runner, dir, remote, branch string) (bool, error) { + // The conventional default names count without consulting the remote. + // This is the safe direction (it can only block a push, never permit + // one), and it keeps the guard meaningful with no network at all. + if branch == "main" || branch == "master" { + return true, nil + } lookupCtx, cancel := context.WithTimeout(ctx, isDefaultBranchRemoteLookupTimeout) defer cancel() if out, err := gitOutput(lookupCtx, runGit, dir, "ls-remote", "--symref", remote, "HEAD"); err == nil { @@ -622,11 +632,24 @@ func isDefaultBranch(ctx context.Context, runGit Runner, dir, remote, branch str if strings.HasPrefix(line, "ref: refs/heads/") && strings.HasSuffix(line, "\tHEAD") { symref := strings.TrimPrefix(line, "ref: refs/heads/") symref = strings.TrimSuffix(symref, "\tHEAD") - return branch == symref + return branch == symref, nil } } } - return branch == "main" || branch == "master" + // The remote lookup failed (unreachable, slow, or gave no symref): fall + // back to the local record of the remote's default branch, written by + // clone or `git remote set-head`. It needs no network, so a slow remote + // cannot degrade the answer. + if out, err := gitOutput(ctx, runGit, dir, "symbolic-ref", "--quiet", "refs/remotes/"+remote+"/HEAD"); err == nil { + if name, ok := strings.CutPrefix(strings.TrimSpace(out), "refs/remotes/"+remote+"/"); ok && name != "" { + return branch == name, nil + } + } + // Fail closed: before this, a lookup timeout silently downgraded the + // check to the main/master name heuristic, so a repository whose default + // is trunk/develop lost the confirmation guard exactly when the remote + // was slow. + return false, fmt.Errorf("default branch for remote %q is unknown (remote lookup failed and no local refs/remotes/%s/HEAD record exists; run `git remote set-head %s --auto` to record it)", remote, remote, remote) } // DefaultBranchOptions resolves whether a branch is the repository's @@ -641,18 +664,22 @@ type DefaultBranchOptions struct { // IsDefaultBranch reports whether options.Branch (or, if empty, the current // branch) is the repository's default/protected branch, using the same check // Push already applies before refusing to push straight to it. It returns the -// resolved branch name alongside the bool so callers that left Branch empty -// don't need a second lookup. -func IsDefaultBranch(ctx context.Context, options DefaultBranchOptions) (bool, string, error) { +// resolved branch name and remote alongside the bool so callers that left +// them empty don't need a second lookup: the remote is resolved exactly the +// way Push resolves it (explicit option, then the branch's configured +// upstream, then "origin"), so a caller can thread the same remote through a +// later Push instead of letting a freshly created branch with no tracking +// configuration silently fall back to "origin". +func IsDefaultBranch(ctx context.Context, options DefaultBranchOptions) (bool, string, string, error) { cwd, err := resolveCwd(options.Cwd) if err != nil { - return false, "", err + return false, "", "", err } runGit, _ := resolveRunners(options.RunGit, nil) root, err := gitOutput(ctx, runGit, cwd, "rev-parse", "--show-toplevel") if err != nil { - return false, "", fmt.Errorf("not a git repository: %w", err) + return false, "", "", fmt.Errorf("not a git repository: %w", err) } root = filepath.Clean(root) @@ -660,14 +687,22 @@ func IsDefaultBranch(ctx context.Context, options DefaultBranchOptions) (bool, s if branch == "" { branch, err = gitOutput(ctx, runGit, root, "rev-parse", "--abbrev-ref", "HEAD") if err != nil { - return false, "", fmt.Errorf("resolve current branch: %w", err) + return false, "", "", fmt.Errorf("resolve current branch: %w", err) } } remote := strings.TrimSpace(options.Remote) if remote == "" { - remote = "origin" + if upstream, err := gitOutput(ctx, runGit, root, "config", "branch."+branch+".remote"); err == nil && upstream != "" { + remote = upstream + } else { + remote = "origin" + } } - return isDefaultBranch(ctx, runGit, root, remote, branch), branch, nil + isDefault, err := isDefaultBranch(ctx, runGit, root, remote, branch) + if err != nil { + return false, branch, remote, err + } + return isDefault, branch, remote, nil } // BranchOptions configures creating and checking out a new local branch. @@ -706,15 +741,23 @@ func CreateBranch(ctx context.Context, options BranchOptions) (BranchResult, err if options.DryRun { return BranchResult{Branch: name}, nil } - // A repeated run (e.g. the same diff producing the same slug, or a retry - // after a prior push under this name) can target a branch that already - // exists locally. `checkout -b` would fail on that collision, so check - // first and check it out directly instead of erroring. - if _, err := gitOutput(ctx, runGit, root, "rev-parse", "--verify", "--quiet", "refs/heads/"+name); err == nil { - if _, err := gitOutput(ctx, runGit, root, "checkout", name); err != nil { - return BranchResult{}, fmt.Errorf("checkout existing branch %q: %w", name, err) + // A repeated run (the same diff producing the same slug, or a low-entropy + // fallback name) can collide with a branch that already exists locally. + // Never check that existing ref out: its history may be entirely + // unrelated to the current work (an old push under the same name), and + // switching to it would publish the stale branch while leaving the new + // commit behind on the default branch. Pick a unique suffixed name off + // the current HEAD instead, and fail visibly when the namespace is + // exhausted rather than guess. + base := name + for suffix := 2; ; suffix++ { + if _, err := gitOutput(ctx, runGit, root, "rev-parse", "--verify", "--quiet", "refs/heads/"+name); err != nil { + break } - return BranchResult{Branch: name}, nil + if suffix > 9 { + return BranchResult{}, fmt.Errorf("branch %q already exists (as do %s-2 through %s-9); delete the stale branches or create one explicitly with `git checkout -b`", base, base, base) + } + name = fmt.Sprintf("%s-%d", base, suffix) } if _, err := gitOutput(ctx, runGit, root, "checkout", "-b", name); err != nil { return BranchResult{}, fmt.Errorf("create branch %q: %w", name, err) @@ -722,6 +765,18 @@ func CreateBranch(ctx context.Context, options BranchOptions) (BranchResult, err return BranchResult{Branch: name}, nil } +// HeadCommitSubject returns the subject line of the HEAD commit, or "" when +// it cannot be resolved (empty repository, not a git directory). Callers use +// it to name the branch for a push that follows a commit, where the working +// tree is already clean and a diff-based name would be empty. +func HeadCommitSubject(ctx context.Context, cwd string, runGit Runner) string { + runGit, _ = resolveRunners(runGit, nil) + if subject, err := gitOutput(ctx, runGit, cwd, "log", "-1", "--format=%s"); err == nil { + return strings.TrimSpace(subject) + } + return "" +} + // CurrentGitUser resolves an identity to prefix generated branch names with: // git config user.name, falling back to the OS account username, falling // back to the literal "user" so BuildBranchName always gets a non-empty diff --git a/internal/zerogit/zerogit_test.go b/internal/zerogit/zerogit_test.go index 56b0b085f..ffb60c612 100644 --- a/internal/zerogit/zerogit_test.go +++ b/internal/zerogit/zerogit_test.go @@ -531,7 +531,7 @@ func TestPushBranchesToRemote(t *testing.T) { {Stdout: root + "\n"}, {Stdout: "feat/some-feature\n"}, {Stdout: "origin\n"}, // config branch.feat/some-feature.remote - {}, // ls-remote --symref (no match → falls through) + {Stdout: "ref: refs/heads/main\tHEAD\nabc123\tHEAD\n"}, // ls-remote --symref: default is main {Stdout: "Everything up-to-date\n"}, }} @@ -558,7 +558,7 @@ func TestPushBranchesToRemote(t *testing.T) { {Stdout: root + "\n"}, {Stdout: "feat/some-feature\n"}, {Stdout: "origin\n"}, // config branch.feat/some-feature.remote - {}, // ls-remote --symref (no match) + {Stdout: "ref: refs/heads/main\tHEAD\nabc123\tHEAD\n"}, // ls-remote --symref: default is main {Stdout: "Everything up-to-date\n"}, }} @@ -645,7 +645,7 @@ func TestPushBranchesToRemote(t *testing.T) { {Stdout: root + "\n"}, {Stdout: "feat/some-feature\n"}, {ExitCode: 1, Stderr: "error: no such section"}, // config lookup fails - {}, // ls-remote --symref (no match) + {Stdout: "ref: refs/heads/main\tHEAD\nabc123\tHEAD\n"}, // ls-remote --symref: default is main {Stdout: "Everything up-to-date\n"}, }} @@ -757,12 +757,18 @@ func TestCreateBranch(t *testing.T) { } }) - t.Run("ChecksOutExistingBranchInsteadOfFailing", func(t *testing.T) { + t.Run("SuffixesNameInsteadOfCheckingOutExistingBranch", func(t *testing.T) { + // An existing branch under the generated name may hold entirely + // unrelated history (an earlier push under the same low-entropy + // name). Checking it out would publish that stale branch and leave + // the new commit behind on the default branch, so CreateBranch must + // pick a fresh suffixed name at the current HEAD instead. root := t.TempDir() runner := &fakeRunner{results: []CommandResult{ {Stdout: root + "\n"}, - {Stdout: "abc1234\n"}, // rev-parse --verify: branch already exists locally - {Stdout: "Switched to branch 'alice/fix-typo'\n"}, + {Stdout: "abc1234\n"}, // rev-parse --verify: alice/fix-typo already exists + {ExitCode: 1}, // rev-parse --verify: alice/fix-typo-2 is free + {Stdout: "Switched to a new branch 'alice/fix-typo-2'\n"}, }} result, err := CreateBranch(context.Background(), BranchOptions{ @@ -773,11 +779,29 @@ func TestCreateBranch(t *testing.T) { if err != nil { t.Fatalf("CreateBranch returned error: %v", err) } - if result.Branch != "alice/fix-typo" { + if result.Branch != "alice/fix-typo-2" { t.Fatalf("unexpected branch: %#v", result) } - if got := runner.commandLine(2); got != "git checkout alice/fix-typo" { - t.Fatalf("expected a plain checkout of the existing branch, got %q", got) + if got := runner.commandLine(3); got != "git checkout -b alice/fix-typo-2" { + t.Fatalf("expected a fresh suffixed branch, got %q", got) + } + }) + + t.Run("FailsVisiblyWhenSuffixNamespaceExhausted", func(t *testing.T) { + root := t.TempDir() + results := []CommandResult{{Stdout: root + "\n"}} + for i := 0; i < 9; i++ { + results = append(results, CommandResult{Stdout: "abc1234\n"}) // every candidate exists + } + runner := &fakeRunner{results: results} + + _, err := CreateBranch(context.Background(), BranchOptions{ + Cwd: root, + Name: "alice/fix-typo", + RunGit: runner.Run, + }) + if err == nil || !strings.Contains(err.Error(), "already exists") { + t.Fatalf("expected a visible exhaustion error, got %v", err) } }) @@ -839,34 +863,39 @@ func TestCreateBranch(t *testing.T) { } func TestIsDefaultBranch(t *testing.T) { - t.Run("ResolvesCurrentBranchAndFallsBackToHeuristic", func(t *testing.T) { + t.Run("ResolvesCurrentBranchByConventionalName", func(t *testing.T) { root := t.TempDir() runner := &fakeRunner{results: []CommandResult{ {Stdout: root + "\n"}, {Stdout: "main\n"}, - {}, // ls-remote --symref (no match → heuristic fallback) + {ExitCode: 1}, // config branch.main.remote unset → origin }} - isDefault, branch, err := IsDefaultBranch(context.Background(), DefaultBranchOptions{ + isDefault, branch, remote, err := IsDefaultBranch(context.Background(), DefaultBranchOptions{ Cwd: root, RunGit: runner.Run, }) if err != nil { t.Fatalf("IsDefaultBranch returned error: %v", err) } - if !isDefault || branch != "main" { - t.Fatalf("unexpected result: isDefault=%v branch=%q", isDefault, branch) + if !isDefault || branch != "main" || remote != "origin" { + t.Fatalf("unexpected result: isDefault=%v branch=%q remote=%q", isDefault, branch, remote) } }) - t.Run("ExplicitNonDefaultBranch", func(t *testing.T) { + t.Run("ResolvesRemoteFromBranchUpstream", func(t *testing.T) { + // A fork setup where the current branch tracks "upstream" must + // resolve and report that remote, not "origin": callers thread it + // into Push so a freshly created feature branch (which has no + // tracking configuration yet) still targets the right remote. root := t.TempDir() runner := &fakeRunner{results: []CommandResult{ {Stdout: root + "\n"}, - {}, // ls-remote --symref (no match → heuristic fallback) + {Stdout: "upstream\n"}, // config branch.feat/some-feature.remote + {Stdout: "ref: refs/heads/main\tHEAD\nabc123\tHEAD\n"}, // ls-remote --symref against upstream }} - isDefault, branch, err := IsDefaultBranch(context.Background(), DefaultBranchOptions{ + isDefault, branch, remote, err := IsDefaultBranch(context.Background(), DefaultBranchOptions{ Cwd: root, Branch: "feat/some-feature", RunGit: runner.Run, @@ -874,8 +903,58 @@ func TestIsDefaultBranch(t *testing.T) { if err != nil { t.Fatalf("IsDefaultBranch returned error: %v", err) } - if isDefault || branch != "feat/some-feature" { - t.Fatalf("unexpected result: isDefault=%v branch=%q", isDefault, branch) + if isDefault || branch != "feat/some-feature" || remote != "upstream" { + t.Fatalf("unexpected result: isDefault=%v branch=%q remote=%q", isDefault, branch, remote) + } + if got := runner.commandLine(2); got != "git ls-remote --symref upstream HEAD" { + t.Fatalf("expected lookup against the resolved remote, got %q", got) + } + }) + + t.Run("FallsBackToLocalRemoteHeadRecord", func(t *testing.T) { + // When the remote lookup fails (offline, slow), the local + // refs/remotes//HEAD record answers without a network. + root := t.TempDir() + runner := &fakeRunner{results: []CommandResult{ + {Stdout: root + "\n"}, + {ExitCode: 1}, // config lookup fails → origin + {ExitCode: 128, Stderr: "fatal:"}, // ls-remote fails + {Stdout: "refs/remotes/origin/trunk\n"}, // local record: default is trunk + }} + + isDefault, branch, remote, err := IsDefaultBranch(context.Background(), DefaultBranchOptions{ + Cwd: root, + Branch: "trunk", + RunGit: runner.Run, + }) + if err != nil { + t.Fatalf("IsDefaultBranch returned error: %v", err) + } + if !isDefault || branch != "trunk" || remote != "origin" { + t.Fatalf("unexpected result: isDefault=%v branch=%q remote=%q", isDefault, branch, remote) + } + }) + + t.Run("FailsClosedWhenDefaultBranchUnknown", func(t *testing.T) { + // Before this, a lookup timeout silently downgraded the check to the + // main/master name heuristic, so a repository whose default is trunk + // lost the confirmation guard exactly when the remote was slow. An + // unknown default must now surface as an error, not as "not default". + root := t.TempDir() + runner := &fakeRunner{results: []CommandResult{ + {Stdout: root + "\n"}, + {ExitCode: 1}, // config lookup fails → origin + {ExitCode: 128, Stderr: "fatal:"}, // ls-remote fails + {ExitCode: 1}, // no local refs/remotes/origin/HEAD record + }} + + _, _, _, err := IsDefaultBranch(context.Background(), DefaultBranchOptions{ + Cwd: root, + Branch: "trunk", + RunGit: runner.Run, + }) + if err == nil || !strings.Contains(err.Error(), "default branch for remote") { + t.Fatalf("expected fail-closed error, got %v", err) } }) } From 68bcce31587c337dd2edb4c1e86f1ab7d12767ad Mon Sep 17 00:00:00 2001 From: euxaristia <25621994+euxaristia@users.noreply.github.com> Date: Wed, 15 Jul 2026 09:09:24 -0400 Subject: [PATCH 4/5] style(zerogit): align fake-runner result comments to gofmt output --- internal/zerogit/zerogit_test.go | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/internal/zerogit/zerogit_test.go b/internal/zerogit/zerogit_test.go index ffb60c612..97c0f35d6 100644 --- a/internal/zerogit/zerogit_test.go +++ b/internal/zerogit/zerogit_test.go @@ -530,7 +530,7 @@ func TestPushBranchesToRemote(t *testing.T) { runner := &fakeRunner{results: []CommandResult{ {Stdout: root + "\n"}, {Stdout: "feat/some-feature\n"}, - {Stdout: "origin\n"}, // config branch.feat/some-feature.remote + {Stdout: "origin\n"}, // config branch.feat/some-feature.remote {Stdout: "ref: refs/heads/main\tHEAD\nabc123\tHEAD\n"}, // ls-remote --symref: default is main {Stdout: "Everything up-to-date\n"}, }} @@ -557,7 +557,7 @@ func TestPushBranchesToRemote(t *testing.T) { runner := &fakeRunner{results: []CommandResult{ {Stdout: root + "\n"}, {Stdout: "feat/some-feature\n"}, - {Stdout: "origin\n"}, // config branch.feat/some-feature.remote + {Stdout: "origin\n"}, // config branch.feat/some-feature.remote {Stdout: "ref: refs/heads/main\tHEAD\nabc123\tHEAD\n"}, // ls-remote --symref: default is main {Stdout: "Everything up-to-date\n"}, }} @@ -644,7 +644,7 @@ func TestPushBranchesToRemote(t *testing.T) { runner := &fakeRunner{results: []CommandResult{ {Stdout: root + "\n"}, {Stdout: "feat/some-feature\n"}, - {ExitCode: 1, Stderr: "error: no such section"}, // config lookup fails + {ExitCode: 1, Stderr: "error: no such section"}, // config lookup fails {Stdout: "ref: refs/heads/main\tHEAD\nabc123\tHEAD\n"}, // ls-remote --symref: default is main {Stdout: "Everything up-to-date\n"}, }} @@ -917,8 +917,8 @@ func TestIsDefaultBranch(t *testing.T) { root := t.TempDir() runner := &fakeRunner{results: []CommandResult{ {Stdout: root + "\n"}, - {ExitCode: 1}, // config lookup fails → origin - {ExitCode: 128, Stderr: "fatal:"}, // ls-remote fails + {ExitCode: 1}, // config lookup fails → origin + {ExitCode: 128, Stderr: "fatal:"}, // ls-remote fails {Stdout: "refs/remotes/origin/trunk\n"}, // local record: default is trunk }} From f6f2a6c1bfb0fbfffc3270bf091b84b8bb847b09 Mon Sep 17 00:00:00 2001 From: euxaristia <25621994+euxaristia@users.noreply.github.com> Date: Wed, 15 Jul 2026 14:51:36 -0400 Subject: [PATCH 5/5] test(zerogit): cover Push's fail-closed path when the default branch is unknown --- internal/zerogit/zerogit_test.go | 22 ++++++++++++++++++++++ 1 file changed, 22 insertions(+) diff --git a/internal/zerogit/zerogit_test.go b/internal/zerogit/zerogit_test.go index 97c0f35d6..e54f42439 100644 --- a/internal/zerogit/zerogit_test.go +++ b/internal/zerogit/zerogit_test.go @@ -665,6 +665,28 @@ func TestPushBranchesToRemote(t *testing.T) { t.Fatalf("unexpected push command: %q", got) } }) + + t.Run("FailsWhenDefaultBranchCannotBeVerified", func(t *testing.T) { + // Push's own fail-closed path: the remote lookup fails and no local + // refs/remotes//HEAD record exists, so Push must refuse with + // guidance instead of pushing an unverifiable branch. + root := t.TempDir() + runner := &fakeRunner{results: []CommandResult{ + {Stdout: root + "\n"}, + {Stdout: "feat/some-feature\n"}, + {Stdout: "origin\n"}, // config branch.feat/some-feature.remote + {ExitCode: 128, Stderr: "fatal:"}, // ls-remote fails + {ExitCode: 1}, // no local refs/remotes/origin/HEAD record + }} + + _, err := Push(context.Background(), PushOptions{ + Cwd: root, + RunGit: runner.Run, + }) + if err == nil || !strings.Contains(err.Error(), "use --yes to override") { + t.Fatalf("expected fail-closed error, got %v", err) + } + }) } func TestCreatePRCommandConstruction(t *testing.T) {