diff --git a/internal/cli/app.go b/internal/cli/app.go index 4d6b11aab..864170e6a 100644 --- a/internal/cli/app.go +++ b/internal/cli/app.go @@ -89,6 +89,22 @@ 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, string, error) + currentGitUser func(context.Context, string) string + headCommitSubject func(context.Context, string) string + commitsAhead func(context.Context, string, string, string) (int, error) + isUnbornRemote func(context.Context, string, string) (bool, error) + refreshTrackingRef func(context.Context, string, string, string) error + branchHasUpstream func(context.Context, string, string) (bool, error) + branchUpstreamRemote func(context.Context, string, string) string + branchUpstreamRef func(context.Context, string, string) string + remoteHasBranch func(context.Context, string, string, string) (bool, error) + currentGitBranch func(context.Context, string) string + deleteBranch func(context.Context, string, string, string) error + resetBranchRef func(context.Context, string, string, string) error + markGeneratedBranch func(context.Context, string, string) error + isGeneratedBranch func(context.Context, string, string) bool runTUI func(context.Context, tui.Options) int runEditor func(string) error checkUpdate func(context.Context, update.Options) (update.Result, error) @@ -195,11 +211,55 @@ 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) + }, + headCommitSubject: func(ctx context.Context, cwd string) string { + return zerogit.HeadCommitSubject(ctx, cwd, nil) + }, + commitsAhead: func(ctx context.Context, cwd, remote, branch string) (int, error) { + return zerogit.CommitsAhead(ctx, cwd, remote, branch, nil) + }, + isUnbornRemote: func(ctx context.Context, cwd, remote string) (bool, error) { + return zerogit.IsUnbornRemote(ctx, cwd, remote, nil) + }, + refreshTrackingRef: func(ctx context.Context, cwd, remote, branch string) error { + return zerogit.RefreshTrackingRef(ctx, cwd, remote, branch, nil) + }, + branchHasUpstream: func(ctx context.Context, cwd, branch string) (bool, error) { + return zerogit.HasUpstream(ctx, cwd, branch, nil) + }, + branchUpstreamRemote: func(ctx context.Context, cwd, branch string) string { + return zerogit.UpstreamRemote(ctx, cwd, branch, nil) + }, + branchUpstreamRef: func(ctx context.Context, cwd, branch string) string { + return zerogit.UpstreamRef(ctx, cwd, branch, nil) + }, + remoteHasBranch: func(ctx context.Context, cwd, remote, branch string) (bool, error) { + return zerogit.RemoteHasBranch(ctx, cwd, remote, branch, nil) + }, + currentGitBranch: func(ctx context.Context, cwd string) string { + return zerogit.CurrentBranch(ctx, cwd, nil) + }, + deleteBranch: func(ctx context.Context, cwd, fallbackBranch, branchToDelete string) error { + return zerogit.DeleteBranch(ctx, cwd, fallbackBranch, branchToDelete, nil) + }, + resetBranchRef: func(ctx context.Context, cwd, branch, newTip string) error { + return zerogit.ResetBranchRef(ctx, cwd, branch, newTip, nil) + }, + markGeneratedBranch: func(ctx context.Context, cwd, branch string) error { + return zerogit.MarkGeneratedBranch(ctx, cwd, branch, nil) + }, + isGeneratedBranch: func(ctx context.Context, cwd, branch string) bool { + return zerogit.IsGeneratedBranch(ctx, cwd, branch, nil) + }, + runTUI: tui.Run, + runEditor: openEditor, + checkUpdate: update.Check, + applyUpdate: update.Apply, + now: time.Now, } } @@ -556,6 +616,54 @@ 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.headCommitSubject == nil { + deps.headCommitSubject = defaults.headCommitSubject + } + if deps.commitsAhead == nil { + deps.commitsAhead = defaults.commitsAhead + } + if deps.isUnbornRemote == nil { + deps.isUnbornRemote = defaults.isUnbornRemote + } + if deps.refreshTrackingRef == nil { + deps.refreshTrackingRef = defaults.refreshTrackingRef + } + if deps.branchHasUpstream == nil { + deps.branchHasUpstream = defaults.branchHasUpstream + } + if deps.branchUpstreamRemote == nil { + deps.branchUpstreamRemote = defaults.branchUpstreamRemote + } + if deps.branchUpstreamRef == nil { + deps.branchUpstreamRef = defaults.branchUpstreamRef + } + if deps.remoteHasBranch == nil { + deps.remoteHasBranch = defaults.remoteHasBranch + } + if deps.currentGitBranch == nil { + deps.currentGitBranch = defaults.currentGitBranch + } + // deleteBranch and resetBranchRef stay nil when unset so unit tests that + // mock createBranch without a real git tree do not hit real restore/delete. + if deps.markGeneratedBranch == nil { + if deps.createBranch != nil { + deps.markGeneratedBranch = func(context.Context, string, string) error { return nil } + } else { + deps.markGeneratedBranch = defaults.markGeneratedBranch + } + } + if deps.isGeneratedBranch == nil { + deps.isGeneratedBranch = defaults.isGeneratedBranch + } if deps.runTUI == nil { deps.runTUI = defaults.runTUI } diff --git a/internal/cli/workflow_test.go b/internal/cli/workflow_test.go index 75b844f47..5aea700db 100644 --- a/internal/cli/workflow_test.go +++ b/internal/cli/workflow_test.go @@ -6,6 +6,7 @@ import ( "encoding/json" "errors" "os" + "os/exec" "path/filepath" "strings" "testing" @@ -20,6 +21,18 @@ import ( "github.com/Gitlawb/zero/internal/zeroruntime" ) +// featureBranchInspect returns an inspectChanges stub for ensureFeatureBranch: +// a clean working tree when BaseRef is empty, and the given committed range +// summary when BaseRef is set (the remote/branch naming path). +func featureBranchInspect(files []zerogit.FileChange, diff string) func(context.Context, zerogit.InspectOptions) (zerogit.ChangeSummary, error) { + return func(ctx context.Context, options zerogit.InspectOptions) (zerogit.ChangeSummary, error) { + if strings.TrimSpace(options.BaseRef) == "" { + return zerogit.ChangeSummary{Clean: true}, nil + } + return zerogit.ChangeSummary{Files: files, Diff: diff}, nil + } +} + func TestRunWorktreesPrepareTextAndJSON(t *testing.T) { cwd := t.TempDir() base := t.TempDir() @@ -803,3 +816,1591 @@ 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, false, 0, appDeps{ + isDefaultBranch: func(ctx context.Context, options zerogit.DefaultBranchOptions) (bool, string, string, error) { + return true, "main", "origin", nil + }, + commitsAhead: func(ctx context.Context, cwd, remote, branch string) (int, error) { return 1, nil }, + refreshTrackingRef: func(ctx context.Context, cwd, remote, branch string) error { return nil }, + inspectChanges: featureBranchInspect([]zerogit.FileChange{{Path: "README.md", Status: "modified"}}, ""), + 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, true, 0, appDeps{ + isDefaultBranch: func(ctx context.Context, options zerogit.DefaultBranchOptions) (bool, string, string, error) { + return true, "main", "origin", nil + }, + commitsAhead: func(ctx context.Context, cwd, remote, branch string) (int, error) { return 1, nil }, + inspectChanges: featureBranchInspect([]zerogit.FileChange{{Path: "login.go", Status: "added"}}, "+func Login() {}"), + 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 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, true, 0, appDeps{ + isDefaultBranch: func(ctx context.Context, options zerogit.DefaultBranchOptions) (bool, string, string, error) { + return true, "main", "origin", nil + }, + commitsAhead: func(ctx context.Context, cwd, remote, branch string) (int, error) { return 1, nil }, + inspectChanges: featureBranchInspect([]zerogit.FileChange{{Path: "login.go", Status: "added"}}, "+func Login() {}"), + 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 TestExtractBranchSlug(t *testing.T) { + // The prompt asks for "ONLY the raw slug", but models add a preamble line, + // wrap the answer in a code fence, or quote a multi-word phrase. The real + // slug must be recovered rather than slugified whole (which would turn a + // preamble into the branch name) or dropped (a bare fence line slugifies to + // nothing). + for _, tc := range []struct { + name string + in string + want string + }{ + {"RawSlug", "add-login-page", "add-login-page"}, + {"Preamble", "Here is a suggested branch name:\nadd-login-page", "add-login-page"}, + {"PreambleWithMultiWord", "Here is a suggested branch name:\nadd login page", "add login page"}, + {"PreambleInlineWithColon", "Here is a suggested branch name: add login page", "add login page"}, + {"InlineLabeledSlug", "Branch name: add-login-page", "add-login-page"}, + // One-word acknowledgements match the slug regex; preamble filter must + // run before the early return so the real suggestion on the next line wins. + {"OneWordAckThenSpaced", "Sure\nadd login page", "add login page"}, + {"OneWordAckThenKebab", "Certainly\nadd-login-page", "add-login-page"}, + {"CodeFence", "```\nadd-login-page\n```", "add-login-page"}, + {"FencedWithLanguage", "```text\nadd-login-page\n```", "add-login-page"}, + {"QuotedPhrase", "\n\"add login page\"\n", "add login page"}, + {"EmptyResponse", " \n\n ", ""}, + } { + t.Run(tc.name, func(t *testing.T) { + if got := extractBranchSlug(tc.in); got != tc.want { + t.Fatalf("extractBranchSlug(%q) = %q, want %q", tc.in, got, tc.want) + } + }) + } +} + +func TestEnsureFeatureBranchExtractsSlugFromMessyLLMReplies(t *testing.T) { + // End-to-end: a preamble line or a code fence around the slug must still + // yield the intended branch name, not one derived from the preamble or a + // silent fallback when the fence line slugifies to nothing. + for _, tc := range []struct { + name string + response string + }{ + {"Preamble", "Here is a suggested branch name:\nadd-login-page"}, + {"PreambleWithMultiWord", "Here is a suggested branch name:\nadd login page"}, + {"CodeFence", "```\nadd-login-page\n```"}, + {"FencedWithLanguage", "```text\nadd-login-page\n```"}, + } { + t.Run(tc.name, func(t *testing.T) { + cwd := t.TempDir() + mockProv := &mockCommitMsgProvider{response: tc.response} + var createdName string + + branch, _, _, err := ensureFeatureBranch(context.Background(), &bytes.Buffer{}, false, cwd, "", false, false, true, 0, appDeps{ + isDefaultBranch: func(ctx context.Context, options zerogit.DefaultBranchOptions) (bool, string, string, error) { + return true, "main", "origin", nil + }, + commitsAhead: func(ctx context.Context, cwd, remote, branch string) (int, error) { return 1, nil }, + inspectChanges: featureBranchInspect([]zerogit.FileChange{{Path: "login.go", Status: "added"}}, "+func Login() {}"), + 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, _, created, err := ensureFeatureBranch(context.Background(), &bytes.Buffer{}, false, cwd, "", false, false, false, 0, appDeps{ + isDefaultBranch: func(ctx context.Context, options zerogit.DefaultBranchOptions) (bool, string, string, error) { + return false, "feat/existing", "origin", nil + }, + branchHasUpstream: func(ctx context.Context, cwd, branch string) (bool, error) { + // An ordinary, already-published feature branch: no lease needed. + return true, 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 created { + t.Fatal("expected an already-published branch not to require the nonexistence lease") + } + 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, false, 0, appDeps{ + isDefaultBranch: func(ctx context.Context, options zerogit.DefaultBranchOptions) (bool, string, 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 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, 0, appDeps{ + isDefaultBranch: func(ctx context.Context, options zerogit.DefaultBranchOptions) (bool, string, string, error) { + return true, "main", "origin", nil + }, + commitsAhead: func(ctx context.Context, cwd, remote, branch string) (int, error) { return 1, nil }, + inspectChanges: featureBranchInspect(nil, ""), // clean tree + empty committed range: name from HEAD + 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, 0, appDeps{ + isDefaultBranch: func(ctx context.Context, options zerogit.DefaultBranchOptions) (bool, string, string, error) { + return true, "main", "origin", nil + }, + commitsAhead: func(ctx context.Context, cwd, remote, branch string) (int, error) { return 1, nil }, + inspectChanges: featureBranchInspect([]zerogit.FileChange{{Path: "login.go", Status: "added"}}, "+func Login() {}"), + 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 TestEnsureFeatureBranchThreadsDiffBytesToInspect(t *testing.T) { + // --diff-bytes caps how much of the diff Inspect returns; with --auto that + // diff is embedded in the provider request, so the cap must reach Inspect + // or a user bounding the proprietary source sent for LLM naming would still + // upload the complete diff. + cwd := t.TempDir() + var gotMaxDiffBytes int + mockProv := &mockCommitMsgProvider{ + response: "feat/capped-diff-branch", + } + + _, _, _, err := ensureFeatureBranch(context.Background(), &bytes.Buffer{}, false, cwd, "", false, false, true, 4096, appDeps{ + isDefaultBranch: func(ctx context.Context, options zerogit.DefaultBranchOptions) (bool, string, string, error) { + return true, "main", "origin", nil + }, + commitsAhead: func(ctx context.Context, cwd, remote, branch string) (int, error) { return 1, nil }, + inspectChanges: func(ctx context.Context, options zerogit.InspectOptions) (zerogit.ChangeSummary, error) { + if strings.TrimSpace(options.BaseRef) == "" { + return zerogit.ChangeSummary{Clean: true}, nil + } + gotMaxDiffBytes = options.MaxDiffBytes + return zerogit.ChangeSummary{ + Files: []zerogit.FileChange{{Path: "README.md", Status: "modified"}}, + Diff: "diff --git a/README.md b/README.md\n+capped diff content", + }, 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) { + return zerogit.BranchResult{Branch: options.Name}, nil + }, + }) + if err != nil { + t.Fatalf("ensureFeatureBranch returned error: %v", err) + } + if gotMaxDiffBytes != 4096 { + t.Fatalf("expected MaxDiffBytes 4096 threaded into Inspect, got %d", gotMaxDiffBytes) + } + if len(mockProv.req.Messages) == 0 || !strings.Contains(mockProv.req.Messages[len(mockProv.req.Messages)-1].Content, "capped diff content") { + t.Fatalf("expected provider request to contain capped diff content, got %#v", mockProv.req) + } +} + +func TestEnsureFeatureBranchRefusesWhenNothingToPublish(t *testing.T) { + // On a clean, up-to-date default branch HEAD is not ahead of the remote + // default, so a push would publish nothing. ensureFeatureBranch must refuse + // instead of creating and pushing an empty feature branch. + cwd := t.TempDir() + createBranchCalled := false + + _, _, _, err := ensureFeatureBranch(context.Background(), &bytes.Buffer{}, false, cwd, "", false, false, false, 0, appDeps{ + isDefaultBranch: func(ctx context.Context, options zerogit.DefaultBranchOptions) (bool, string, string, error) { + return true, "main", "origin", nil + }, + commitsAhead: func(ctx context.Context, cwd, remote, branch string) (int, error) { + return 0, nil + }, + inspectChanges: func(ctx context.Context, options zerogit.InspectOptions) (zerogit.ChangeSummary, error) { + if strings.TrimSpace(options.BaseRef) != "" { + t.Fatal("base-ref inspect should not run when there is nothing to publish") + } + return zerogit.ChangeSummary{Clean: true}, nil + }, + createBranch: func(ctx context.Context, options zerogit.BranchOptions) (zerogit.BranchResult, error) { + createBranchCalled = true + return zerogit.BranchResult{Branch: options.Name}, nil + }, + }) + if err == nil || !strings.Contains(err.Error(), "no changes to publish") { + t.Fatalf("expected a no-changes-to-publish error, got %v", err) + } + if createBranchCalled { + t.Fatal("expected createBranch not to be called when nothing is publishable") + } +} + +func TestEnsureFeatureBranchRefusesDirtyWorkingTree(t *testing.T) { + // CreateBranch/Push publish commits only. With an ahead commit plus + // uncommitted edits, naming and pushing would leave those edits behind + // under a branch/PR that does not include them. + cwd := t.TempDir() + createBranchCalled := false + + _, _, _, err := ensureFeatureBranch(context.Background(), &bytes.Buffer{}, false, cwd, "", false, false, false, 0, appDeps{ + isDefaultBranch: func(ctx context.Context, options zerogit.DefaultBranchOptions) (bool, string, string, error) { + return true, "main", "origin", nil + }, + commitsAhead: func(ctx context.Context, cwd, remote, branch string) (int, error) { + t.Fatal("commitsAhead should not run when the working tree is dirty") + return 0, nil + }, + inspectChanges: func(ctx context.Context, options zerogit.InspectOptions) (zerogit.ChangeSummary, error) { + if strings.TrimSpace(options.BaseRef) != "" { + t.Fatal("base-ref inspect should not run when the working tree is dirty") + } + return zerogit.ChangeSummary{ + Clean: false, + Files: []zerogit.FileChange{{Path: "wip.go", Status: "modified"}}, + }, nil + }, + createBranch: func(ctx context.Context, options zerogit.BranchOptions) (zerogit.BranchResult, error) { + createBranchCalled = true + return zerogit.BranchResult{Branch: options.Name}, nil + }, + }) + if err == nil || !strings.Contains(err.Error(), "uncommitted changes") { + t.Fatalf("expected an uncommitted-changes error, got %v", err) + } + if createBranchCalled { + t.Fatal("expected createBranch not to be called when the working tree is dirty") + } +} + +func TestEnsureFeatureBranchFailsWhenAheadCountUnknown(t *testing.T) { + // A missing remote-tracking ref (never fetched) means the ahead count + // cannot be determined. Fail closed rather than guessing that there is + // something to publish (or naming a branch from a working-tree snapshot). + cwd := t.TempDir() + createBranchCalled := false + + _, _, _, err := ensureFeatureBranch(context.Background(), &bytes.Buffer{}, false, cwd, "", false, false, false, 0, appDeps{ + isDefaultBranch: func(ctx context.Context, options zerogit.DefaultBranchOptions) (bool, string, string, error) { + return true, "main", "origin", nil + }, + commitsAhead: func(ctx context.Context, cwd, remote, branch string) (int, error) { + return 0, errors.New("unknown revision origin/main") + }, + isUnbornRemote: func(ctx context.Context, cwd, remote string) (bool, error) { + // Not unborn: the remote exists and has branches, it just was + // never fetched locally. This must still fail closed. + return false, nil + }, + inspectChanges: func(ctx context.Context, options zerogit.InspectOptions) (zerogit.ChangeSummary, error) { + if strings.TrimSpace(options.BaseRef) != "" { + t.Fatal("base-ref inspect should not run when the ahead count is unknown") + } + return zerogit.ChangeSummary{Clean: true}, nil + }, + createBranch: func(ctx context.Context, options zerogit.BranchOptions) (zerogit.BranchResult, error) { + createBranchCalled = true + return zerogit.BranchResult{Branch: options.Name}, nil + }, + }) + if err == nil || !strings.Contains(err.Error(), "cannot determine whether HEAD is ahead") { + t.Fatalf("expected an ahead-count-unknown error, got %v", err) + } + if createBranchCalled { + t.Fatal("expected createBranch not to be called when the publishable range is unknown") + } +} + +func TestEnsureFeatureBranchFailsWhenUnbornCheckErrors(t *testing.T) { + // The ahead count is unknown AND the unborn probe itself fails (remote + // unreachable): this must still fail closed exactly like the plain + // unknown-ahead-count case, not be treated as a confirmed-unborn remote. + cwd := t.TempDir() + createBranchCalled := false + + _, _, _, err := ensureFeatureBranch(context.Background(), &bytes.Buffer{}, false, cwd, "", false, false, false, 0, appDeps{ + isDefaultBranch: func(ctx context.Context, options zerogit.DefaultBranchOptions) (bool, string, string, error) { + return true, "main", "origin", nil + }, + commitsAhead: func(ctx context.Context, cwd, remote, branch string) (int, error) { + return 0, errors.New("unknown revision origin/main") + }, + isUnbornRemote: func(ctx context.Context, cwd, remote string) (bool, error) { + return false, errors.New("remote unreachable") + }, + inspectChanges: func(ctx context.Context, options zerogit.InspectOptions) (zerogit.ChangeSummary, error) { + if strings.TrimSpace(options.BaseRef) != "" { + t.Fatal("base-ref inspect should not run when the ahead count is unknown") + } + return zerogit.ChangeSummary{Clean: true}, nil + }, + createBranch: func(ctx context.Context, options zerogit.BranchOptions) (zerogit.BranchResult, error) { + createBranchCalled = true + return zerogit.BranchResult{Branch: options.Name}, nil + }, + }) + if err == nil || !strings.Contains(err.Error(), "cannot determine whether HEAD is ahead") { + t.Fatalf("expected an ahead-count-unknown error, got %v", err) + } + if createBranchCalled { + t.Fatal("expected createBranch not to be called when the unborn state is unconfirmed") + } +} + +// TestEnsureFeatureBranchRefusesUnbornRemote covers jatmn's P1: auto-creating +// only a feature branch on a brand-new empty remote leaves remote HEAD +// dangling (no default branch), so the next `changes pr` fails closed and a +// later `push --yes` of main publishes the same tip with no PR diff. Refuse +// auto-branching until the initial default branch is established with --yes. +func TestEnsureFeatureBranchRefusesUnbornRemote(t *testing.T) { + cwd := t.TempDir() + createBranchCalled := false + var isUnbornRemoteCalled bool + + _, _, created, err := ensureFeatureBranch(context.Background(), &bytes.Buffer{}, false, cwd, "", false, false, false, 0, appDeps{ + isDefaultBranch: func(ctx context.Context, options zerogit.DefaultBranchOptions) (bool, string, string, error) { + return true, "main", "origin", nil + }, + inspectChanges: featureBranchInspect(nil, ""), + commitsAhead: func(ctx context.Context, cwd, remote, branch string) (int, error) { + return 0, errors.New("unknown revision origin/main..HEAD") + }, + isUnbornRemote: func(ctx context.Context, cwd, remote string) (bool, error) { + isUnbornRemoteCalled = true + if remote != "origin" { + t.Fatalf("expected the resolved remote %q, got %q", "origin", remote) + } + return true, nil + }, + createBranch: func(ctx context.Context, options zerogit.BranchOptions) (zerogit.BranchResult, error) { + createBranchCalled = true + return zerogit.BranchResult{Branch: options.Name}, nil + }, + }) + if err == nil || !strings.Contains(err.Error(), "no branches yet") || !strings.Contains(err.Error(), "--yes") { + t.Fatalf("expected unborn-remote refuse error mentioning --yes, got %v", err) + } + if !isUnbornRemoteCalled { + t.Fatal("expected isUnbornRemote to be consulted after commitsAhead failed") + } + if created || createBranchCalled { + t.Fatal("expected no feature branch on an unborn remote") + } +} + +func TestEnsureFeatureBranchInspectsAgainstResolvedRemoteBranch(t *testing.T) { + // Push and CreateBranch only publish commits, so the branch (and, with + // --auto, the diff sent to a provider) must be named from what HEAD is + // actually ahead of the resolved remote branch by, not from the working + // tree: Inspect must be asked to diff against "/", the + // same ref commitsAhead just checked. + cwd := t.TempDir() + var gotBaseRef string + + _, _, _, err := ensureFeatureBranch(context.Background(), &bytes.Buffer{}, false, cwd, "", false, false, false, 0, appDeps{ + isDefaultBranch: func(ctx context.Context, options zerogit.DefaultBranchOptions) (bool, string, string, error) { + return true, "main", "origin", nil + }, + commitsAhead: func(ctx context.Context, cwd, remote, branch string) (int, error) { return 1, nil }, + inspectChanges: func(ctx context.Context, options zerogit.InspectOptions) (zerogit.ChangeSummary, error) { + if strings.TrimSpace(options.BaseRef) == "" { + return zerogit.ChangeSummary{Clean: true}, nil + } + gotBaseRef = options.BaseRef + 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 + }, + }) + if err != nil { + t.Fatalf("ensureFeatureBranch returned error: %v", err) + } + if gotBaseRef != "origin/main" { + t.Fatalf("expected Inspect to diff against %q, got %q", "origin/main", gotBaseRef) + } +} + +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 + }, + commitsAhead: func(ctx context.Context, cwd, remote, branch string) (int, error) { return 1, nil }, + refreshTrackingRef: func(ctx context.Context, cwd, remote, branch string) error { return nil }, + inspectChanges: featureBranchInspect([]zerogit.FileChange{{Path: "README.md", Status: "modified"}}, ""), + 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 + var requiredNewRemoteBranch bool + + 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", "origin", nil + }, + commitsAhead: func(ctx context.Context, cwd, remote, branch string) (int, error) { return 1, nil }, + refreshTrackingRef: func(ctx context.Context, cwd, remote, branch string) error { return nil }, + inspectChanges: featureBranchInspect([]zerogit.FileChange{{Path: "README.md", Status: "modified"}}, ""), + 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 + requiredNewRemoteBranch = options.RequireNewRemoteBranch + 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()) + } + // CreateBranch's own remote-collision probe runs before this push, so the + // push itself must assert the destination is still new, closing the + // window for a concurrent creator of the same name. + if !requiredNewRemoteBranch { + t.Fatal("expected Push to require the destination not already exist on the remote") + } +} + +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, string, error) { + return true, "main", "origin", nil + }, + commitsAhead: func(ctx context.Context, cwd, remote, branch string) (int, error) { return 1, nil }, + refreshTrackingRef: func(ctx context.Context, cwd, remote, branch string) error { return nil }, + inspectChanges: featureBranchInspect([]zerogit.FileChange{{Path: "README.md", Status: "modified"}}, ""), + 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 + + 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, string, error) { + isDefaultBranchCalled = true + return true, "main", "origin", 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) + } + if options.RequireNewRemoteBranch { + t.Fatal("expected RequireNewRemoteBranch to be false: no branch was created") + } + 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") + } +} + +func TestRunChangesPRRejectsUnbornRemoteEvenWithYes(t *testing.T) { + cwd := t.TempDir() + isUnbornRemoteCalled := false + + var stdout, stderr bytes.Buffer + exitCode := runWithDeps([]string{"changes", "pr", "--yes", "--fill"}, &stdout, &stderr, appDeps{ + getwd: func() (string, error) { return cwd, nil }, + isUnbornRemote: func(ctx context.Context, cwd, remote string) (bool, error) { + isUnbornRemoteCalled = true + if remote != "origin" { + t.Fatalf("expected remote %q, got %q", "origin", remote) + } + return true, nil + }, + pushChanges: func(ctx context.Context, options zerogit.PushOptions) (zerogit.PushResult, error) { + t.Fatal("pushChanges should not be called on unborn remote") + return zerogit.PushResult{}, nil + }, + createPR: func(ctx context.Context, options zerogit.PROptions) (zerogit.PRResult, error) { + t.Fatal("createPR should not be called on unborn remote") + return zerogit.PRResult{}, nil + }, + }) + + if exitCode != exitUsage { + t.Fatalf("expected exit code %d, got %d", exitUsage, exitCode) + } + if !isUnbornRemoteCalled { + t.Fatal("expected isUnbornRemote to be consulted even when --yes is passed") + } + if !strings.Contains(stderr.String(), "cannot create pull request on unborn remote origin") { + t.Fatalf("unexpected stderr: %q", stderr.String()) + } +} + +// TestRunChangesPushPreservesLeaseOnRetryAfterCollision covers jatmn's P1 +// finding: the initial auto-branch push lost a force-with-lease race against +// a concurrent creator of the same branch name, but the local generated +// branch is left checked out with no successful push behind it. A retry of +// `changes push` takes ensureFeatureBranch's non-default early return (it's +// already off the default branch), and must still require the destination +// not already exist on the remote - dropping that requirement would let the +// retry silently fast-forward whatever the concurrent creator published. +func TestRunChangesPushPreservesLeaseOnRetryAfterCollision(t *testing.T) { + cwd := t.TempDir() + var requireNewRemoteBranch bool + var pushedBranch string + createBranchCalled := false + + 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) { + // HEAD is already on the branch generated by the previous, + // lease-rejected attempt. + return false, "someone/readme-md", "origin", nil + }, + branchUpstreamRef: func(ctx context.Context, cwd, branch string) string { + if branch != "someone/readme-md" { + t.Fatalf("unexpected branch queried for upstream: %q", branch) + } + // The first push never landed (it lost the lease), so this + // branch has no published upstream for origin/someone/readme-md. + return "" + }, + remoteHasBranch: func(ctx context.Context, cwd, remote, branch string) (bool, error) { + // Concurrent creator may have published the name, but this case + // models the pure lease-reject path where the remote still lacks + // this branch under our identity; keep the nonexistence lease. + return false, nil + }, + isGeneratedBranch: func(ctx context.Context, cwd, branch string) bool { + return true + }, + createBranch: func(ctx context.Context, options zerogit.BranchOptions) (zerogit.BranchResult, error) { + createBranchCalled = true + return zerogit.BranchResult{}, nil + }, + pushChanges: func(ctx context.Context, options zerogit.PushOptions) (zerogit.PushResult, error) { + pushedBranch = options.Branch + requireNewRemoteBranch = options.RequireNewRemoteBranch + 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 createBranchCalled { + t.Fatal("expected createBranch not to be called on a retry already off the default branch") + } + if pushedBranch != "someone/readme-md" { + t.Fatalf("expected the retry to target the same generated branch, got %q", pushedBranch) + } + if !requireNewRemoteBranch { + t.Fatal("expected the retry to preserve the nonexistence lease (RequireNewRemoteBranch)") + } +} + +// TestRunChangesPushDoesNotRequireLeaseForAlreadyPublishedBranch is the +// companion case: a generated branch whose exact / was +// published by push -u must not reassert the nonexistence lease. +func TestRunChangesPushDoesNotRequireLeaseForAlreadyPublishedBranch(t *testing.T) { + cwd := t.TempDir() + var requireNewRemoteBranch bool + + 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 false, "someone/readme-md", "origin", nil + }, + isGeneratedBranch: func(ctx context.Context, cwd, branch string) bool { + return true + }, + branchUpstreamRef: func(ctx context.Context, cwd, branch string) string { + return "origin/someone/readme-md" + }, + pushChanges: func(ctx context.Context, options zerogit.PushOptions) (zerogit.PushResult, error) { + requireNewRemoteBranch = options.RequireNewRemoteBranch + 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 requireNewRemoteBranch { + t.Fatal("expected an already-published branch not to require the nonexistence lease") + } +} + +// TestRunChangesPushDropsLeaseWhenRemoteExistsWithoutLocalUpstream covers the +// push -u config-write race: the first push published origin/ but left +// local upstream empty. A later push (after another commit) must not reassert +// --force-with-lease=: against that existing remote ref. +func TestRunChangesPushDropsLeaseWhenRemoteExistsWithoutLocalUpstream(t *testing.T) { + cwd := t.TempDir() + var requireNewRemoteBranch bool + var remoteProbed bool + + 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 false, "someone/readme-md", "origin", nil + }, + isGeneratedBranch: func(ctx context.Context, cwd, branch string) bool { + return true + }, + branchUpstreamRef: func(ctx context.Context, cwd, branch string) string { + // Local config write failed after the first push -u. + return "" + }, + remoteHasBranch: func(ctx context.Context, cwd, remote, branch string) (bool, error) { + remoteProbed = true + if remote != "origin" || branch != "someone/readme-md" { + t.Fatalf("unexpected remote probe: %s/%s", remote, branch) + } + return true, nil + }, + pushChanges: func(ctx context.Context, options zerogit.PushOptions) (zerogit.PushResult, error) { + requireNewRemoteBranch = options.RequireNewRemoteBranch + 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 !remoteProbed { + t.Fatal("expected remoteHasBranch to be consulted when local upstream is missing") + } + if requireNewRemoteBranch { + t.Fatal("expected no nonexistence lease when the remote branch already exists") + } +} + +// TestRunChangesPushKeepsLeaseWhenRemoteMissingAndLocalUpstreamEmpty is the +// collision-retry companion: local upstream empty and remote has no branch +// still needs the nonexistence lease. +func TestRunChangesPushKeepsLeaseWhenRemoteMissingAndLocalUpstreamEmpty(t *testing.T) { + cwd := t.TempDir() + var requireNewRemoteBranch bool + + 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 false, "someone/readme-md", "origin", nil + }, + isGeneratedBranch: func(ctx context.Context, cwd, branch string) bool { + return true + }, + branchUpstreamRef: func(ctx context.Context, cwd, branch string) string { + return "" + }, + remoteHasBranch: func(ctx context.Context, cwd, remote, branch string) (bool, error) { + return false, nil + }, + pushChanges: func(ctx context.Context, options zerogit.PushOptions) (zerogit.PushResult, error) { + requireNewRemoteBranch = options.RequireNewRemoteBranch + 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 !requireNewRemoteBranch { + t.Fatal("expected nonexistence lease when remote branch is still missing") + } +} + +// TestRunChangesPushPreservesLeaseWhenInheritedOriginRemoteConfig covers the +// autoSetupMerge=inherit race: branch.remote equals origin before any push, +// but the upstream ref is still origin/main. Lease must stay. +func TestRunChangesPushPreservesLeaseWhenInheritedOriginRemoteConfig(t *testing.T) { + cwd := t.TempDir() + var requireNewRemoteBranch bool + + 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 false, "someone/readme-md", "origin", nil + }, + isGeneratedBranch: func(ctx context.Context, cwd, branch string) bool { + return true + }, + branchUpstreamRemote: func(ctx context.Context, cwd, branch string) string { + // Misleading: inherit copies remote=origin from main. + return "origin" + }, + branchUpstreamRef: func(ctx context.Context, cwd, branch string) string { + return "origin/main" + }, + remoteHasBranch: func(ctx context.Context, cwd, remote, branch string) (bool, error) { + // Feature branch not published yet; only main exists remotely. + return false, nil + }, + pushChanges: func(ctx context.Context, options zerogit.PushOptions) (zerogit.PushResult, error) { + requireNewRemoteBranch = options.RequireNewRemoteBranch + 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 !requireNewRemoteBranch { + t.Fatal("expected lease kept when only an inherited origin/main upstream exists") + } +} + +// TestEnsureFeatureBranchRefreshesTrackingRefBeforeCheckingAhead covers +// jatmn's P2 finding: validate publishability against the live default, not a +// stale local tracking ref. IsDefaultBranch already contacted the remote for +// its symref check, but a merely-cached origin/main (last fetched earlier) +// can sit behind the remote's live tip. Before the fix, commitsAhead ran +// straight against that stale ref: this simulates the stale ref reporting +// zero commits ahead until the tracking ref is refreshed, matching the +// scenario where the remote has already advanced past what was last fetched. +func TestEnsureFeatureBranchRefreshesTrackingRefBeforeCheckingAhead(t *testing.T) { + cwd := t.TempDir() + refreshed := false + var createdName string + + branch, _, _, err := ensureFeatureBranch(context.Background(), &bytes.Buffer{}, false, cwd, "", false, false, false, 0, appDeps{ + isDefaultBranch: func(ctx context.Context, options zerogit.DefaultBranchOptions) (bool, string, string, error) { + return true, "main", "origin", nil + }, + refreshTrackingRef: func(ctx context.Context, cwd, remote, branch string) error { + if remote != "origin" || branch != "main" { + t.Fatalf("unexpected refresh target: %s/%s", remote, branch) + } + refreshed = true + return nil + }, + commitsAhead: func(ctx context.Context, cwd, remote, branch string) (int, error) { + if !refreshed { + // The stale, unrefreshed local origin/main is behind and + // would report zero commits ahead even though HEAD carries + // real, unpublished work relative to the remote's live tip. + return 0, nil + } + return 2, nil + }, + inspectChanges: featureBranchInspect([]zerogit.FileChange{{Path: "README.md", Status: "modified"}}, ""), + 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 !refreshed { + t.Fatal("expected the tracking ref to be refreshed before the ahead check") + } + if branch != "someone/readme-md" || createdName != "someone/readme-md" { + t.Fatalf("unexpected branch: got %q (created %q)", branch, createdName) + } +} + +// TestEnsureFeatureBranchFailsClosedWhenTrackingRefCannotBeRefreshed asserts +// that a failed refresh is never silently ignored in favor of trusting +// whatever commitsAhead reports against the (potentially stale, unrefreshed) +// local ref: the command must fail closed instead of risking a false "no +// changes to publish" from comparing against a tracking ref that could not be +// confirmed fresh. +func TestEnsureFeatureBranchFailsClosedWhenTrackingRefCannotBeRefreshed(t *testing.T) { + cwd := t.TempDir() + createBranchCalled := false + + _, _, _, err := ensureFeatureBranch(context.Background(), &bytes.Buffer{}, false, cwd, "", false, false, false, 0, appDeps{ + isDefaultBranch: func(ctx context.Context, options zerogit.DefaultBranchOptions) (bool, string, string, error) { + return true, "main", "origin", nil + }, + refreshTrackingRef: func(ctx context.Context, cwd, remote, branch string) error { + return errors.New("remote unreachable") + }, + commitsAhead: func(ctx context.Context, cwd, remote, branch string) (int, error) { + return 0, nil + }, + isUnbornRemote: func(ctx context.Context, cwd, remote string) (bool, error) { + return false, nil + }, + inspectChanges: func(ctx context.Context, options zerogit.InspectOptions) (zerogit.ChangeSummary, error) { + return zerogit.ChangeSummary{Clean: true}, nil + }, + createBranch: func(ctx context.Context, options zerogit.BranchOptions) (zerogit.BranchResult, error) { + createBranchCalled = true + return zerogit.BranchResult{Branch: options.Name}, nil + }, + }) + if err == nil || !strings.Contains(err.Error(), "cannot determine whether HEAD is ahead") { + t.Fatalf("expected a fail-closed error when the tracking ref could not be refreshed, got %v", err) + } + if createBranchCalled { + t.Fatal("expected createBranch not to be called when the tracking ref could not be refreshed") + } +} + +func TestEnsureFeatureBranchRequiresLeaseWhenPushedToDifferentRemote(t *testing.T) { + cwd := t.TempDir() + branch, _, requireNew, err := ensureFeatureBranch(context.Background(), &bytes.Buffer{}, false, cwd, "upstream", false, false, false, 0, appDeps{ + isDefaultBranch: func(ctx context.Context, options zerogit.DefaultBranchOptions) (bool, string, string, error) { + return false, "user/feature", "origin", nil + }, + isGeneratedBranch: func(ctx context.Context, cwd, branch string) bool { + return true + }, + // Already published to origin/user/feature, but this push targets upstream. + branchUpstreamRef: func(ctx context.Context, cwd, branch string) string { + return "origin/user/feature" + }, + }) + if err != nil { + t.Fatalf("ensureFeatureBranch: %v", err) + } + if branch != "user/feature" { + t.Fatalf("branch = %q, want user/feature", branch) + } + if !requireNew { + t.Fatal("expected requireNewRemoteBranch to be true when target remote differs from published upstream") + } +} + +// TestEnsureFeatureBranchKeepsLeaseWhenUpstreamIsInheritedMain covers +// branch.autoSetupMerge=inherit: checkout -b copies remote=origin and +// merge=refs/heads/main, so branch@{upstream} is origin/main even though +// origin/user/slug was never published. The lease must stay. +func TestEnsureFeatureBranchKeepsLeaseWhenUpstreamIsInheritedMain(t *testing.T) { + cwd := t.TempDir() + _, _, requireNew, err := ensureFeatureBranch(context.Background(), &bytes.Buffer{}, false, cwd, "", false, false, false, 0, appDeps{ + isDefaultBranch: func(ctx context.Context, options zerogit.DefaultBranchOptions) (bool, string, string, error) { + return false, "user/slug", "origin", nil + }, + isGeneratedBranch: func(ctx context.Context, cwd, branch string) bool { + return true + }, + branchUpstreamRef: func(ctx context.Context, cwd, branch string) string { + // Inherited from main via autoSetupMerge=inherit, not a real push -u. + return "origin/main" + }, + }) + if err != nil { + t.Fatalf("ensureFeatureBranch: %v", err) + } + if !requireNew { + t.Fatal("expected lease kept when upstream is inherited origin/main, not origin/user/slug") + } +} + +// TestEnsureFeatureBranchDropsLeaseOnlyAfterExactPublishedRef: after a real +// push -u origin user/slug, retry must not reassert the nonexistence lease. +func TestEnsureFeatureBranchDropsLeaseOnlyAfterExactPublishedRef(t *testing.T) { + cwd := t.TempDir() + _, _, requireNew, err := ensureFeatureBranch(context.Background(), &bytes.Buffer{}, false, cwd, "", false, false, false, 0, appDeps{ + isDefaultBranch: func(ctx context.Context, options zerogit.DefaultBranchOptions) (bool, string, string, error) { + return false, "user/slug", "origin", nil + }, + isGeneratedBranch: func(ctx context.Context, cwd, branch string) bool { + return true + }, + branchUpstreamRef: func(ctx context.Context, cwd, branch string) string { + return "origin/user/slug" + }, + }) + if err != nil { + t.Fatalf("ensureFeatureBranch: %v", err) + } + if requireNew { + t.Fatal("expected lease dropped after exact origin/user/slug was published") + } +} + +// TestEnsureFeatureBranchKeepsLeaseAfterLostRace: force-with-lease rejected +// the first push; no upstream for the generated branch exists yet. +func TestEnsureFeatureBranchKeepsLeaseAfterLostRace(t *testing.T) { + cwd := t.TempDir() + _, _, requireNew, err := ensureFeatureBranch(context.Background(), &bytes.Buffer{}, false, cwd, "", false, false, false, 0, appDeps{ + isDefaultBranch: func(ctx context.Context, options zerogit.DefaultBranchOptions) (bool, string, string, error) { + return false, "user/slug", "origin", nil + }, + isGeneratedBranch: func(ctx context.Context, cwd, branch string) bool { + return true + }, + branchUpstreamRef: func(ctx context.Context, cwd, branch string) string { + return "" + }, + }) + if err != nil { + t.Fatalf("ensureFeatureBranch: %v", err) + } + if !requireNew { + t.Fatal("expected lease kept after a lost force-with-lease race with no published upstream") + } +} + +func TestEnsureFeatureBranchRollsBackOnMarkGeneratedBranchFailure(t *testing.T) { + cwd := t.TempDir() + deletedBranch := "" + _, _, _, err := ensureFeatureBranch(context.Background(), &bytes.Buffer{}, false, cwd, "", false, false, false, 0, appDeps{ + isDefaultBranch: func(ctx context.Context, options zerogit.DefaultBranchOptions) (bool, string, string, error) { + return true, "main", "origin", nil + }, + commitsAhead: func(ctx context.Context, cwd, remote, branch string) (int, error) { + return 1, nil + }, + inspectChanges: featureBranchInspect([]zerogit.FileChange{{Path: "README.md", Status: "modified"}}, ""), + currentGitUser: func(ctx context.Context, cwd string) string { return "user" }, + createBranch: func(ctx context.Context, options zerogit.BranchOptions) (zerogit.BranchResult, error) { + return zerogit.BranchResult{Branch: options.Name}, nil + }, + markGeneratedBranch: func(ctx context.Context, cwd, branch string) error { + return errors.New("config lock error") + }, + deleteBranch: func(ctx context.Context, cwd, fallbackBranch, branchToDelete string) error { + deletedBranch = branchToDelete + return nil + }, + }) + if err == nil || !strings.Contains(err.Error(), "config lock error") { + t.Fatalf("expected mark error, got %v", err) + } + if deletedBranch != "user/readme-md" { + t.Fatalf("expected rollback deletion of user/readme-md, got %q", deletedBranch) + } +} + +func TestRunChangesPRYesBypassesDefaultBranchCheck(t *testing.T) { + cwd := t.TempDir() + var stdout, stderr bytes.Buffer + exitCode := runWithDeps([]string{"changes", "pr", "--yes", "--fill"}, &stdout, &stderr, appDeps{ + getwd: func() (string, error) { return cwd, nil }, + isDefaultBranch: func(ctx context.Context, options zerogit.DefaultBranchOptions) (bool, string, string, error) { + return false, "", "", errors.New("remote unreachable") + }, + pushChanges: func(ctx context.Context, options zerogit.PushOptions) (zerogit.PushResult, error) { + return zerogit.PushResult{Remote: "origin", Branch: "main"}, 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 with --yes even when isDefaultBranch errors, got %d: %s", exitSuccess, exitCode, stderr.String()) + } +} + +// TestEnsureFeatureBranchRestoresDefaultBranchAfterCreate covers jatmn's P2 +// finding: after committing on main and auto-creating user/slug at the same +// HEAD, the local default branch must be moved back to its remote-tracking +// tip so it does not keep the new commit (which would diverge after a +// squash-merge). +func TestEnsureFeatureBranchRestoresDefaultBranchAfterCreate(t *testing.T) { + cwd := t.TempDir() + var resetBranch, resetTip string + var createdName string + + branch, _, created, err := ensureFeatureBranch(context.Background(), &bytes.Buffer{}, false, cwd, "", false, false, false, 0, appDeps{ + isDefaultBranch: func(ctx context.Context, options zerogit.DefaultBranchOptions) (bool, string, string, error) { + return true, "main", "origin", nil + }, + commitsAhead: func(ctx context.Context, cwd, remote, branch string) (int, error) { return 1, nil }, + refreshTrackingRef: func(ctx context.Context, cwd, remote, branch string) error { return nil }, + inspectChanges: featureBranchInspect([]zerogit.FileChange{{Path: "README.md", Status: "modified"}}, ""), + 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 + }, + resetBranchRef: func(ctx context.Context, cwd, branch, newTip string) error { + resetBranch = branch + resetTip = newTip + return nil + }, + }) + if err != nil { + t.Fatalf("ensureFeatureBranch returned error: %v", err) + } + if !created || branch != createdName { + t.Fatalf("expected a created branch, got branch=%q created=%v", branch, created) + } + if resetBranch != "main" || resetTip != "origin/main" { + t.Fatalf("expected reset of main to origin/main, got branch=%q tip=%q", resetBranch, resetTip) + } +} + +// TestEnsureFeatureBranchRollsBackWhenDefaultRestoreFails covers the +// failure-safe path: if restoring the default branch ref fails after the +// feature branch was created, delete the feature branch and surface the error +// rather than leaving a half-moved tree. +func TestEnsureFeatureBranchRollsBackWhenDefaultRestoreFails(t *testing.T) { + cwd := t.TempDir() + deletedBranch := "" + + _, _, _, err := ensureFeatureBranch(context.Background(), &bytes.Buffer{}, false, cwd, "", false, false, false, 0, appDeps{ + isDefaultBranch: func(ctx context.Context, options zerogit.DefaultBranchOptions) (bool, string, string, error) { + return true, "main", "origin", nil + }, + commitsAhead: func(ctx context.Context, cwd, remote, branch string) (int, error) { return 1, nil }, + inspectChanges: featureBranchInspect([]zerogit.FileChange{{Path: "README.md", Status: "modified"}}, ""), + currentGitUser: func(ctx context.Context, cwd string) string { return "user" }, + createBranch: func(ctx context.Context, options zerogit.BranchOptions) (zerogit.BranchResult, error) { + return zerogit.BranchResult{Branch: options.Name}, nil + }, + resetBranchRef: func(ctx context.Context, cwd, branch, newTip string) error { + return errors.New("update-ref failed") + }, + deleteBranch: func(ctx context.Context, cwd, fallbackBranch, branchToDelete string) error { + if fallbackBranch != "main" { + t.Fatalf("expected fallback main, got %q", fallbackBranch) + } + deletedBranch = branchToDelete + return nil + }, + }) + if err == nil || !strings.Contains(err.Error(), "restore default branch") { + t.Fatalf("expected restore error, got %v", err) + } + if deletedBranch != "user/readme-md" { + t.Fatalf("expected rollback deletion of user/readme-md, got %q", deletedBranch) + } +} + +// TestEnsureFeatureBranchRestoresSourceUpstreamOnForkRemote covers jatmn's P2: +// --remote selects the push destination only. When local main tracks +// origin/main and the user passes --remote upstream, restore must leave main +// at origin/main, not rewrite it to upstream/main. +func TestEnsureFeatureBranchRestoresSourceUpstreamOnForkRemote(t *testing.T) { + cwd := t.TempDir() + var resetBranch, resetTip string + + _, _, created, err := ensureFeatureBranch(context.Background(), &bytes.Buffer{}, false, cwd, "upstream", false, false, false, 0, appDeps{ + isDefaultBranch: func(ctx context.Context, options zerogit.DefaultBranchOptions) (bool, string, string, error) { + if options.Remote != "upstream" { + t.Fatalf("expected requested remote upstream, got %q", options.Remote) + } + return true, "main", "upstream", nil + }, + commitsAhead: func(ctx context.Context, cwd, remote, branch string) (int, error) { return 1, nil }, + refreshTrackingRef: func(ctx context.Context, cwd, remote, branch string) error { return nil }, + inspectChanges: featureBranchInspect([]zerogit.FileChange{{Path: "README.md", Status: "modified"}}, ""), + currentGitUser: func(ctx context.Context, cwd string) string { return "Someone" }, + branchUpstreamRef: func(ctx context.Context, cwd, branch string) string { + if branch != "main" { + t.Fatalf("expected upstream query for main, got %q", branch) + } + // Fork: main tracks origin, not the push destination. + return "origin/main" + }, + createBranch: func(ctx context.Context, options zerogit.BranchOptions) (zerogit.BranchResult, error) { + if options.Remote != "upstream" { + t.Fatalf("expected create against upstream, got %q", options.Remote) + } + return zerogit.BranchResult{Branch: options.Name}, nil + }, + resetBranchRef: func(ctx context.Context, cwd, branch, newTip string) error { + resetBranch = branch + resetTip = newTip + return nil + }, + }) + if err != nil { + t.Fatalf("ensureFeatureBranch returned error: %v", err) + } + if !created { + t.Fatal("expected a feature branch to be created") + } + if resetBranch != "main" || resetTip != "origin/main" { + t.Fatalf("expected restore main -> origin/main (source upstream), got %q -> %q", resetBranch, resetTip) + } +} + +// TestRunChangesPushRestoresDefaultBranchOnAutoBranchPath exercises the +// commit-then-push auto-branch path end-to-end through runChangesPush and +// asserts the default branch is restored to its remote tip after the feature +// branch is created. +func TestRunChangesPushRestoresDefaultBranchOnAutoBranchPath(t *testing.T) { + cwd := t.TempDir() + var resetBranch, resetTip string + 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, string, error) { + return true, "main", "origin", nil + }, + commitsAhead: func(ctx context.Context, cwd, remote, branch string) (int, error) { return 1, nil }, + refreshTrackingRef: func(ctx context.Context, cwd, remote, branch string) error { return nil }, + inspectChanges: featureBranchInspect([]zerogit.FileChange{{Path: "README.md", Status: "modified"}}, ""), + 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 + }, + resetBranchRef: func(ctx context.Context, cwd, branch, newTip string) error { + resetBranch = branch + resetTip = newTip + return 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 resetBranch != "main" || resetTip != "origin/main" { + t.Fatalf("expected default branch restore main -> origin/main, got %q -> %q", resetBranch, resetTip) + } + if pushedBranch != "someone/readme-md" { + t.Fatalf("expected push of auto-created branch, got %q", pushedBranch) + } +} + +// TestRunChangesPRYesUsesActualUpstreamForUnbornCheck covers jatmn's P2 +// finding: with changes pr --yes and no --remote, the unborn-remote preflight +// must resolve the current branch's configured upstream (e.g. a fork's +// "upstream") rather than calling branchUpstreamRemote with an empty branch +// name and falling back to origin. +func TestRunChangesPRYesUsesActualUpstreamForUnbornCheck(t *testing.T) { + cwd := t.TempDir() + var unbornRemoteChecked string + var upstreamBranchArg string + isDefaultBranchCalled := false + + var stdout, stderr bytes.Buffer + exitCode := runWithDeps([]string{"changes", "pr", "--yes", "--fill"}, &stdout, &stderr, appDeps{ + getwd: func() (string, error) { return cwd, nil }, + isDefaultBranch: func(ctx context.Context, options zerogit.DefaultBranchOptions) (bool, string, string, error) { + isDefaultBranchCalled = true + return false, "", "", errors.New("remote HEAD unreachable") + }, + currentGitBranch: func(ctx context.Context, cwd string) string { + return "feature/fork-work" + }, + branchUpstreamRemote: func(ctx context.Context, cwd, branch string) string { + upstreamBranchArg = branch + if branch == "feature/fork-work" { + return "upstream" + } + return "" + }, + isUnbornRemote: func(ctx context.Context, cwd, remote string) (bool, error) { + unbornRemoteChecked = remote + return false, nil + }, + pushChanges: func(ctx context.Context, options zerogit.PushOptions) (zerogit.PushResult, error) { + return zerogit.PushResult{Remote: "upstream", Branch: "feature/fork-work"}, nil + }, + createPR: func(ctx context.Context, options zerogit.PROptions) (zerogit.PRResult, error) { + return zerogit.PRResult{Output: "https://example.invalid/pr/2"}, 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 under --yes") + } + if upstreamBranchArg != "feature/fork-work" { + t.Fatalf("expected branchUpstreamRemote to receive current branch, got %q", upstreamBranchArg) + } + if unbornRemoteChecked != "upstream" { + t.Fatalf("expected unborn check against upstream, got %q", unbornRemoteChecked) + } +} + +// TestRunChangesPRYesRejectsUnbornNonOriginUpstream: when the resolved +// upstream is unborn, the preflight must still block even under --yes. +func TestRunChangesPRYesRejectsUnbornNonOriginUpstream(t *testing.T) { + cwd := t.TempDir() + var stdout, stderr bytes.Buffer + exitCode := runWithDeps([]string{"changes", "pr", "--yes", "--fill"}, &stdout, &stderr, appDeps{ + getwd: func() (string, error) { return cwd, nil }, + currentGitBranch: func(ctx context.Context, cwd string) string { + return "feature/fork-work" + }, + branchUpstreamRemote: func(ctx context.Context, cwd, branch string) string { + return "upstream" + }, + isUnbornRemote: func(ctx context.Context, cwd, remote string) (bool, error) { + if remote != "upstream" { + t.Fatalf("expected remote upstream, got %q", remote) + } + return true, nil + }, + pushChanges: func(ctx context.Context, options zerogit.PushOptions) (zerogit.PushResult, error) { + t.Fatal("pushChanges should not run on unborn upstream") + return zerogit.PushResult{}, nil + }, + }) + if exitCode != exitUsage { + t.Fatalf("expected exit code %d, got %d: %s", exitUsage, exitCode, stderr.String()) + } + if !strings.Contains(stderr.String(), "cannot create pull request on unborn remote upstream") { + t.Fatalf("unexpected stderr: %q", stderr.String()) + } +} + +// TestRunChangesBareRemotePushThenPRUsable is the end-to-end bare-remote +// regression for jatmn's unborn P1: auto-branch must not be the first remote +// ref. Establish main with --yes, then auto-branch a second commit and show +// that changes pr remains usable (push + createPR succeed against a remote +// that has a real default branch). +func TestRunChangesBareRemotePushThenPRUsable(t *testing.T) { + if _, err := exec.LookPath("git"); err != nil { + t.Skipf("git unavailable: %v", err) + } + + tmp := t.TempDir() + bare := filepath.Join(tmp, "remote.git") + repo := filepath.Join(tmp, "repo") + // -b main so remote HEAD points at the default branch GitHub/new repos use; + // otherwise a bare repo's HEAD stays on master and IsDefaultBranch fails + // closed after the first main push (dangling HEAD + existing heads). + runWorkflowGit(t, tmp, "init", "--bare", "-b", "main", bare) + runWorkflowGit(t, tmp, "init", repo) + runWorkflowGit(t, repo, "config", "user.name", "Zero") + runWorkflowGit(t, repo, "config", "user.email", "zero@example.invalid") + runWorkflowGit(t, repo, "checkout", "-b", "main") + writeWorkflowFile(t, filepath.Join(repo, "README.md"), "initial\n") + runWorkflowGit(t, repo, "add", "README.md") + runWorkflowGit(t, repo, "commit", "-m", "Initial commit") + runWorkflowGit(t, repo, "remote", "add", "origin", bare) + + // Auto-branch on an unborn remote must refuse (not publish only a feature branch). + var stdout, stderr bytes.Buffer + exitCode := runWithDeps([]string{"changes", "push"}, &stdout, &stderr, appDeps{ + getwd: func() (string, error) { return repo, nil }, + }) + if exitCode == exitSuccess { + t.Fatalf("expected unborn remote to refuse auto-branch, stdout=%q stderr=%q", stdout.String(), stderr.String()) + } + if !strings.Contains(stderr.String(), "no branches yet") { + t.Fatalf("expected unborn refuse message, got stderr=%q", stderr.String()) + } + + // Establish the default branch. + stdout.Reset() + stderr.Reset() + exitCode = runWithDeps([]string{"changes", "push", "--yes"}, &stdout, &stderr, appDeps{ + getwd: func() (string, error) { return repo, nil }, + }) + if exitCode != exitSuccess { + t.Fatalf("push --yes failed: exit=%d stdout=%q stderr=%q", exitCode, stdout.String(), stderr.String()) + } + heads := runWorkflowGit(t, bare, "show-ref", "--heads") + if !strings.Contains(heads, "refs/heads/main") { + t.Fatalf("expected bare remote to have main after --yes, got %q", heads) + } + + // Second commit: auto-branch + push, then pr against an established remote. + writeWorkflowFile(t, filepath.Join(repo, "feature.txt"), "work\n") + runWorkflowGit(t, repo, "add", "feature.txt") + runWorkflowGit(t, repo, "commit", "-m", "add feature") + + var pushedBranch string + var prCalled bool + stdout.Reset() + stderr.Reset() + deps := defaultAppDeps() + deps.getwd = func() (string, error) { return repo, nil } + deps.currentGitUser = func(ctx context.Context, cwd string) string { return "Someone" } + basePush := deps.pushChanges + deps.pushChanges = func(ctx context.Context, options zerogit.PushOptions) (zerogit.PushResult, error) { + pushedBranch = options.Branch + return basePush(ctx, options) + } + deps.createPR = func(ctx context.Context, options zerogit.PROptions) (zerogit.PRResult, error) { + prCalled = true + return zerogit.PRResult{Output: "https://example.invalid/pr/1"}, nil + } + + exitCode = runWithDeps([]string{"changes", "push"}, &stdout, &stderr, deps) + if exitCode != exitSuccess { + t.Fatalf("auto-branch push failed: exit=%d stdout=%q stderr=%q", exitCode, stdout.String(), stderr.String()) + } + if pushedBranch == "" || pushedBranch == "main" { + t.Fatalf("expected auto-created feature branch push, got %q", pushedBranch) + } + heads = runWorkflowGit(t, bare, "show-ref", "--heads") + if !strings.Contains(heads, "refs/heads/main") { + t.Fatalf("expected main to remain on bare remote, got %q", heads) + } + if !strings.Contains(heads, "refs/heads/"+pushedBranch) { + t.Fatalf("expected feature branch %q on bare remote, got %q", pushedBranch, heads) + } + + stdout.Reset() + stderr.Reset() + exitCode = runWithDeps([]string{"changes", "pr", "--fill"}, &stdout, &stderr, deps) + if exitCode != exitSuccess { + t.Fatalf("changes pr failed after established remote: exit=%d stdout=%q stderr=%q", exitCode, stdout.String(), stderr.String()) + } + if !prCalled { + t.Fatal("expected createPR to run once the default branch exists") + } +} + +func runWorkflowGit(t *testing.T, dir string, args ...string) string { + t.Helper() + cmd := exec.Command("git", args...) + cmd.Dir = dir + out, err := cmd.CombinedOutput() + if err != nil { + t.Fatalf("git %s failed: %v\n%s", strings.Join(args, " "), err, string(out)) + } + return string(out) +} + +func writeWorkflowFile(t *testing.T, path, content string) { + t.Helper() + if err := os.WriteFile(path, []byte(content), 0o600); err != nil { + t.Fatalf("write %s: %v", path, err) + } +} diff --git a/internal/cli/workflows.go b/internal/cli/workflows.go index bc13bc7f8..29c04042e 100644 --- a/internal/cli/workflows.go +++ b/internal/cli/workflows.go @@ -4,6 +4,8 @@ import ( "context" "fmt" "io" + "path/filepath" + "regexp" "strconv" "strings" "time" @@ -522,8 +524,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"} @@ -531,6 +533,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`"} } @@ -838,8 +845,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 `) @@ -862,11 +868,18 @@ func runChangesPush(args []string, stdout io.Writer, stderr io.Writer, deps appD return writeExecUsageError(stderr, err.Error()) } + branch, remote, created, err := ensureFeatureBranch(context.Background(), stdout, options.json, workspaceRoot, options.remote, options.yes, options.dryRun, options.auto, options.maxDiffBytes, 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, + RequireNewRemoteBranch: created, AllowPushDefaultBranch: options.yes, }) if err != nil { @@ -914,6 +927,47 @@ func runChangesPR(args []string, stdout io.Writer, stderr io.Writer, deps appDep return writeExecUsageError(stderr, err.Error()) } + // Resolve the remote the unborn-remote preflight (and later push) will + // target. Explicit --remote wins. Otherwise: + // - without --yes: IsDefaultBranch resolves the current branch's + // upstream (then "origin"), matching Push's remote resolution; + // - with --yes: skip IsDefaultBranch so a remote-HEAD failure cannot + // block the documented default-branch override, but still resolve the + // current branch and its configured upstream so a fork's non-origin + // upstream is not silently replaced by "origin". + targetRemote := strings.TrimSpace(options.remote) + if targetRemote == "" { + if !options.yes { + _, _, remoteForCheck, err := deps.isDefaultBranch(context.Background(), zerogit.DefaultBranchOptions{Cwd: workspaceRoot, Remote: options.remote}) + if err != nil { + return writeExecUsageError(stderr, err.Error()) + } + targetRemote = remoteForCheck + } else { + currentBranch := "" + if deps.currentGitBranch != nil { + currentBranch = deps.currentGitBranch(context.Background(), workspaceRoot) + } + if deps.branchUpstreamRemote != nil && currentBranch != "" { + targetRemote = deps.branchUpstreamRemote(context.Background(), workspaceRoot, currentBranch) + } + } + if targetRemote == "" { + targetRemote = "origin" + } + } + + if deps.isUnbornRemote != nil { + if unborn, unbornErr := deps.isUnbornRemote(context.Background(), workspaceRoot, targetRemote); unbornErr == nil && unborn { + return writeExecUsageError(stderr, fmt.Sprintf("cannot create pull request on unborn remote %s: push the initial default branch first", targetRemote)) + } + } + + branch, remote, created, err := ensureFeatureBranch(context.Background(), stdout, options.json, workspaceRoot, options.remote, options.yes, false, options.auto, options.maxDiffBytes, 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 @@ -921,8 +975,10 @@ 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, + RequireNewRemoteBranch: created, AllowPushDefaultBranch: options.yes, }) if err != nil { @@ -1004,3 +1060,347 @@ 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), plus the remote the +// preflight resolved (requestedRemote, then the original branch's configured +// upstream, then "origin"), plus whether Push should require that branch not +// already exist on that remote. Callers must pass the 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. Callers must also pass that third value +// through as Push's RequireNewRemoteBranch: CreateBranch's own +// remote-collision probe runs before this returns, and closing that race +// requires Push's push itself to assert the destination is still new. That +// requirement also carries across a retry: if the current branch is already +// non-default (this call didn't create it) but has not been successfully +// published to the exact target remote under the same branch name, keep the +// nonexistence lease. branch..remote alone is not enough: with +// branch.autoSetupMerge=inherit, checkout -b copies remote=origin from the +// source branch before any push. After a successful create, the original +// default branch ref is moved back to its own upstream tip (not necessarily +// the push destination) so the feature branch exclusively owns the +// publishable commits; a failed restore rolls the feature branch back. +// allowDefaultBranch (the --yes flag) and dryRun both opt out via the "" +// branch / false 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. maxDiffBytes caps the committed-range diff Inspect +// returns, so a user who passed --diff-bytes to bound the proprietary source +// sent for LLM naming has that cap honored here just as the commit path does. +// The working tree must be clean and HEAD must be ahead of the resolved remote +// default before a branch is created; otherwise the push would either leave +// uncommitted edits behind or publish an empty comparison. A confirmed-unborn +// remote (freshly created, zero refs) is not auto-branched: pushing only a +// feature branch would leave the remote without its configured default +// (dangling HEAD), and the next `changes pr` cannot recover. Establish the +// initial default branch with --yes first, then auto-branch subsequent work. +func ensureFeatureBranch(ctx context.Context, stdout io.Writer, jsonMode bool, workspaceRoot string, requestedRemote string, allowDefaultBranch bool, dryRun bool, autoNaming bool, maxDiffBytes int, deps appDeps) (string, string, bool, error) { + if allowDefaultBranch || dryRun { + return "", strings.TrimSpace(requestedRemote), false, nil + } + + isDefault, currentBranch, remote, err := deps.isDefaultBranch(ctx, zerogit.DefaultBranchOptions{Cwd: workspaceRoot, Remote: requestedRemote}) + if err != nil { + return "", "", false, err + } + if !isDefault { + requireNewRemoteBranch := false + if deps.isGeneratedBranch != nil && deps.isGeneratedBranch(ctx, workspaceRoot, currentBranch) { + // Keep the zero-value lease unless this exact / + // relationship was recorded by a successful push -u. Inherited + // origin/main (autoSetupMerge=inherit) and a push to a different + // remote both leave the lease in place. + // + // Local upstream config alone is not a remote nonexistence proof: + // git push -u can publish the branch and still fail to write + // .git/config. When local config is missing or wrong, probe the + // remote before reasserting --force-with-lease=:. + targetRemote := firstNonEmptyString(requestedRemote, remote) + publishedRef := targetRemote + "/" + currentBranch + upstreamRef := "" + if deps.branchUpstreamRef != nil { + upstreamRef = deps.branchUpstreamRef(ctx, workspaceRoot, currentBranch) + } + if upstreamRef == publishedRef { + requireNewRemoteBranch = false + } else if deps.remoteHasBranch != nil { + exists, existsErr := deps.remoteHasBranch(ctx, workspaceRoot, targetRemote, currentBranch) + if existsErr != nil { + return "", "", false, fmt.Errorf("cannot check whether %s already exists on remote %s: %w", currentBranch, targetRemote, existsErr) + } + requireNewRemoteBranch = !exists + } else { + requireNewRemoteBranch = true + } + } + return currentBranch, remote, requireNewRemoteBranch, nil + } + + // CreateBranch and Push publish commits only. A dirty working tree would + // leave uncommitted edits behind under a branch/PR that does not include + // them, so refuse until the tree is clean (commit or stash first). + workingTree, err := deps.inspectChanges(ctx, zerogit.InspectOptions{Cwd: workspaceRoot}) + if err != nil { + return "", "", false, fmt.Errorf("failed to inspect working tree: %w", err) + } + if !workingTree.Clean { + return "", "", false, fmt.Errorf("working tree has uncommitted changes; commit or stash them before pushing from the default branch") + } + + // Refresh the local remote-tracking ref before trusting it: IsDefaultBranch + // already contacted the remote for its symref check, but the tracking ref + // commitsAhead reads from is only a local cache (written at clone or the + // last fetch) and can sit behind the remote's live tip. Left stale, a real + // publishable range could look like zero commits ahead, and the diff + // derived below (which reuses this same ref as its base) would be stale + // too. A failure here (offline, or a genuinely unborn remote with no such + // ref to fetch yet) is folded into the same fail-closed path as an + // unresolvable ahead count below. + var fetchErr error + if deps.refreshTrackingRef != nil { + fetchErr = deps.refreshTrackingRef(ctx, workspaceRoot, remote, currentBranch) + } + + // Branching off the default branch only makes sense when HEAD carries a + // commit that is not already on the remote default branch. A clean, + // up-to-date default branch would otherwise publish a feature branch at the + // exact default tip. If the ahead count cannot be determined (for example + // the remote-tracking ref was never fetched), fail rather than guess. When + // the remote is confirmed unborn, refuse auto-branching entirely: the + // initial default branch must be established first (changes push --yes) + // so the remote has a real HEAD before any feature branch is published. + ahead, aheadErr := deps.commitsAhead(ctx, workspaceRoot, remote, currentBranch) + if fetchErr != nil || aheadErr != nil { + var unbornRemote bool + var unbornErr error + if deps.isUnbornRemote != nil { + unbornRemote, unbornErr = deps.isUnbornRemote(ctx, workspaceRoot, remote) + } + if unbornErr == nil && unbornRemote { + return "", "", false, fmt.Errorf("remote %s has no branches yet; push the initial default branch first with --yes (`zero changes push --yes`), then use auto-branching for subsequent work", remote) + } + cause := aheadErr + if cause == nil { + cause = fetchErr + } + return "", "", false, fmt.Errorf("cannot determine whether HEAD is ahead of %s/%s: %w; fetch the remote tracking branch first", remote, currentBranch, cause) + } + if ahead == 0 { + return "", "", false, fmt.Errorf("no changes to publish: HEAD is not ahead of %s/%s; commit your work before pushing", remote, currentBranch) + } + + // Name the branch (and, with --auto, send the provider) from what HEAD is + // actually ahead of the resolved remote branch by, using the same ref + // commitsAhead just checked. A working-tree snapshot can describe edits a + // commit-only push will never include. + baseRef := remote + "/" + currentBranch + summary, err := deps.inspectChanges(ctx, zerogit.InspectOptions{Cwd: workspaceRoot, BaseRef: baseRef, MaxDiffBytes: maxDiffBytes}) + if err != nil { + return "", "", false, fmt.Errorf("failed to inspect changes: %w", err) + } + + slug := fallbackBranchSlug(summary) + if len(summary.Files) == 0 { + // An empty commit (or one whose only content the diff omits) leaves + // nothing to derive a slug from; name the branch from the commit + // subject 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 + } + } + } + } + + // Capture the source default branch's own upstream before leaving it. + // --remote selects the push destination; restore must not rewrite local + // main to upstream/main when main still tracks origin/main (fork setups). + restoreTip := "" + if deps.branchUpstreamRef != nil { + restoreTip = deps.branchUpstreamRef(ctx, workspaceRoot, currentBranch) + } + if restoreTip == "" { + restoreTip = remote + "/" + currentBranch + } + + name := zerogit.BuildBranchName(deps.currentGitUser(ctx, workspaceRoot), slug) + result, err := deps.createBranch(ctx, zerogit.BranchOptions{Cwd: workspaceRoot, Name: name, Remote: remote}) + if err != nil { + return "", "", false, fmt.Errorf("failed to create branch: %w", err) + } + if deps.markGeneratedBranch != nil { + if err := deps.markGeneratedBranch(ctx, workspaceRoot, result.Branch); err != nil { + if deps.deleteBranch != nil { + _ = deps.deleteBranch(ctx, workspaceRoot, currentBranch, result.Branch) + } + return "", "", false, fmt.Errorf("failed to mark generated branch: %w", err) + } + } + // The normal flow commits on the default branch first, then creates the + // feature branch at the same HEAD. Without moving the original default + // ref back, local main keeps the pre-squash commits and diverges from + // its upstream after a squash-merge (and a later pull/push can re-publish + // them). Once the feature branch owns those commits, restore the default + // branch to the source branch's own upstream tip captured above. + // Failure-safe: if the restore fails, delete the feature branch and + // check the default branch back out so the tree is not left half-moved. + if deps.resetBranchRef != nil { + if err := deps.resetBranchRef(ctx, workspaceRoot, currentBranch, restoreTip); err != nil { + if deps.deleteBranch != nil { + _ = deps.deleteBranch(ctx, workspaceRoot, currentBranch, result.Branch) + } + return "", "", false, fmt.Errorf("failed to restore default branch %s to %s after auto-branching: %w", currentBranch, restoreTip, err) + } + } + if !jsonMode { + fmt.Fprintf(stdout, "Created branch %s (was on %s)\n", result.Branch, currentBranch) + } + return result.Branch, remote, true, 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(extractBranchSlug(collected.Text)) + if slug == "" { + return "", fmt.Errorf("provider returned empty branch slug") + } + return slug, nil +} + +// slugLineRe matches a line that already reads as a kebab-case slug (letters, +// digits, and internal single hyphens only). extractBranchSlug prefers such a +// line so a preamble sentence is never mistaken for the slug. +var slugLineRe = regexp.MustCompile(`^[a-zA-Z0-9]+(-[a-zA-Z0-9]+)*$`) + +func isPreambleText(s string) bool { + lower := strings.ToLower(strings.TrimSpace(s)) + if strings.HasSuffix(lower, ":") || strings.HasSuffix(lower, ".") || strings.HasSuffix(lower, "!") || strings.HasSuffix(lower, "?") { + return true + } + preambles := []string{ + "here is", "here's", "below is", "below are", "sure", "certainly", + "i suggest", "suggested branch", "branch name", "recommended branch", + "how about", "you could use", + } + for _, p := range preambles { + if strings.HasPrefix(lower, p) { + return true + } + } + return false +} + +// extractBranchSlug pulls the intended slug out of a model response that didn't +// follow the "output only the raw slug" instruction exactly. It drops Markdown +// code-fence lines, then prefers a line that already looks like a kebab-case +// slug: that skips a leading preamble such as "Here is a suggested branch +// name:" in favor of the "add-login-page" line that follows it. One-word +// acknowledgements that happen to match the slug regex ("Sure", "Certainly") +// are filtered as preamble before the early return, so "Sure\nadd login page" +// yields the real suggestion. Inline labeled forms ("Branch name: add-login-page") +// still work: the preamble check runs on the extracted value after the label +// is stripped. If no line is strictly kebab-cased, it prefers a plausible +// non-preamble line so plain multi-word replies still slugify correctly. +func extractBranchSlug(text string) string { + var firstLine string + var plausibleLine string + + for _, line := range strings.Split(text, "\n") { + line = strings.TrimSpace(line) + if strings.HasPrefix(line, "```") { + continue + } + line = strings.TrimSpace(strings.Trim(line, `"'`)) + if line == "" { + continue + } + + candidate := line + if idx := strings.Index(line, ":"); idx != -1 { + after := strings.TrimSpace(strings.Trim(line[idx+1:], `"'`)) + if after != "" { + candidate = after + } + } + + // Preamble classification runs before accepting a slug-shaped line so + // "Sure" does not win over a later "add-login-page". The check is on + // candidate (post label-strip) so "Branch name: add-login-page" still + // returns the extracted slug. + if slugLineRe.MatchString(candidate) && !isPreambleText(candidate) { + return candidate + } + + if firstLine == "" && !isPreambleText(candidate) { + firstLine = candidate + } + + if (!isPreambleText(line) || candidate != line) && !isPreambleText(candidate) { + if plausibleLine == "" { + plausibleLine = candidate + } + } + } + + if plausibleLine != "" { + return plausibleLine + } + return firstLine +} diff --git a/internal/zerogit/zerogit.go b/internal/zerogit/zerogit.go index f7bff7c17..97a43cf9a 100644 --- a/internal/zerogit/zerogit.go +++ b/internal/zerogit/zerogit.go @@ -6,7 +6,10 @@ import ( "fmt" "os" "os/exec" + "os/user" "path/filepath" + "regexp" + "strconv" "strings" "unicode/utf8" @@ -528,11 +531,18 @@ func firstNonEmpty(values ...string) string { } type PushOptions struct { - Cwd string - Remote string - Branch string - Force bool - DryRun bool + Cwd string + Remote string + Branch string + Force bool + DryRun bool + // RequireNewRemoteBranch guards this push with a zero-value + // --force-with-lease, so it is rejected instead of fast-forwarding if + // Branch already exists on Remote. CreateBranch's collision probe reads + // the remote's branches before this push runs, leaving a window in which + // a concurrent creator can publish the same generated name; this closes + // it at the one point that actually talks to the remote atomically. + RequireNewRemoteBranch bool AllowPushDefaultBranch bool RunGit Runner RunGitEnv EnvRunner @@ -578,7 +588,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) } } @@ -587,7 +601,14 @@ func Push(ctx context.Context, options PushOptions) (PushResult, error) { if options.DryRun { args = append(args, "--dry-run") } - if options.Force { + switch { + case options.RequireNewRemoteBranch: + // An empty expected value means the ref must not currently exist on + // the remote: Git rejects the push if another client created + // after CreateBranch's own remote probe ran, instead of + // silently fast-forwarding it with this work. + args = append(args, "--force-with-lease="+branch+":") + case options.Force: args = append(args, "--force-with-lease") } args = append(args, "-u", "--", remote, branch) @@ -597,6 +618,25 @@ func Push(ctx context.Context, options PushOptions) (PushResult, error) { return PushResult{}, fmt.Errorf("push: %w", err) } + // git push -u can create the remote branch and still exit 0 when it cannot + // write branch..remote/merge (for example .git/config.lock held by + // another process). Zero must not report that as a full success: without + // the local upstream, a later ensureFeatureBranch retry would reassert + // --force-with-lease=: against a branch that already exists. + expectedUpstream := remote + "/" + branch + if UpstreamRef(ctx, root, branch, runGit) != expectedUpstream { + if _, setErr := gitOutput(ctx, runGit, root, "branch", "--set-upstream-to="+expectedUpstream, branch); setErr != nil { + return PushResult{Remote: remote, Branch: branch, Output: output}, fmt.Errorf( + "push published %s but failed to configure local upstream %s: %w; run `git branch --set-upstream-to=%s %s` then retry", + expectedUpstream, expectedUpstream, setErr, expectedUpstream, branch) + } + if UpstreamRef(ctx, root, branch, runGit) != expectedUpstream { + return PushResult{Remote: remote, Branch: branch, Output: output}, fmt.Errorf( + "push published %s but local upstream is still not %s; run `git branch --set-upstream-to=%s %s` then retry", + expectedUpstream, expectedUpstream, expectedUpstream, branch) + } + } + return PushResult{ Remote: remote, Branch: branch, @@ -604,18 +644,492 @@ func Push(ctx context.Context, options PushOptions) (PushResult, error) { }, nil } -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 { +func isDefaultBranch(ctx context.Context, runGit Runner, dir, remote, branch string) (bool, error) { + // "--" terminates option parsing: remote comes from --remote/branch config, + // and a value like "--upload-pack=/bin/echo" must reach Git as a positional + // argument, never as an option. + if out, err := gitOutput(ctx, 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") { symref := strings.TrimPrefix(line, "ref: refs/heads/") symref = strings.TrimSuffix(symref, "\tHEAD") - return branch == symref + return branch == symref, nil + } + } + if strings.TrimSpace(out) == "" { + // HEAD didn't resolve to anything. A genuinely unborn remote (no + // refs at all) answers this way, and it cannot have a protected + // default branch yet, so the fail-closed error below would make + // the very first feature-branch push a dead end. But a non-empty + // remote whose HEAD symref is dangling or missing produces the + // same empty output while still possibly having a protected + // default under a name this couldn't identify, so confirm the + // remote truly has no branches before granting the unborn + // exception; a non-default first push is safe. + if heads, headsErr := gitOutput(ctx, runGit, dir, "ls-remote", "--heads", "--", remote); headsErr == nil && strings.TrimSpace(heads) == "" { + if branch == "main" || branch == "master" { + return true, nil + } + return false, nil + } + } + } + // The remote lookup failed (unreachable, slow, or gave no symref): the + // local refs/remotes//HEAD record, written by clone or `git remote + // set-head`, is only a cache. Trust it to *block* a push (a positive match + // proves branch is the recorded default) but never to *clear* the guard. If + // the server renamed its default (main -> trunk) the stale record still + // names main, so a mismatch here is not evidence that pushing trunk is safe; + // fall through instead of returning false. + 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 == branch { + return true, nil + } + } + // The conventional default names are only a fallback for when the remote's + // actual default genuinely could not be determined above (live or cached). + // Applying this before consulting the remote at all would misidentify a + // repository whose real default is e.g. "trunk" but that also happens to + // have a local/tracked "main": the live symref result must win whenever + // it's available. This is still safe-direction only (it can block a push, + // never permit one) since it's the last resort before failing closed. + if branch == "main" || branch == "master" { + return true, 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 +// 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 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 + } + 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 == "" { + if upstream, err := gitOutput(ctx, runGit, root, "config", "branch."+branch+".remote"); err == nil && upstream != "" { + remote = upstream + } else { + remote = "origin" + } + } + // Honor the caller's deadline (or lack of one). A fixed short timeout here + // turned slow but reachable remotes (SSH/VPN handshakes) into fail-closed + // "use --yes" errors for ordinary feature-branch pushes, because + // ensureFeatureBranch always consults this before Push. Callers that need + // a bound should pass a context with a deadline. + 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. +type BranchOptions struct { + Cwd string + Name string // full branch name, e.g. "alice/fix-typo" + DryRun bool + RunGit Runner + // Remote, when non-empty, is the remote the new branch will be pushed + // to; its branch names are treated as taken when resolving collisions, + // so a remote-only stale branch (e.g. an old merged PR) is never + // fast-forwarded with unrelated new work. + Remote string +} + +// 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 + } + // 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. + // + // Local refs are not enough: a branch that exists only on the target + // remote (an old merged-PR branch, or one pruned locally) would be + // silently fast-forwarded by the later `push -u`, appending the new work + // to an unrelated remote branch. Probe the remote's heads once under the + // caller's context (same connectivity the later push needs) and fail + // visibly when the remote cannot be consulted. + remoteTaken := map[string]bool{} + if remote := strings.TrimSpace(options.Remote); remote != "" { + out, err := gitOutput(ctx, runGit, root, "ls-remote", "--heads", "--", remote) + if err != nil { + return BranchResult{}, fmt.Errorf("cannot check branch names against remote %q: %w", remote, err) + } + for _, line := range strings.Split(out, "\n") { + if _, ref, ok := strings.Cut(strings.TrimSpace(line), "\t"); ok { + remoteTaken[strings.TrimPrefix(strings.TrimSpace(ref), "refs/heads/")] = true } } } - return branch == "main" || branch == "master" + base := name + for suffix := 2; ; suffix++ { + _, localErr := gitOutput(ctx, runGit, root, "rev-parse", "--verify", "--quiet", "refs/heads/"+name) + if localErr != nil && !remoteTaken[name] { + break + } + if suffix > 9 { + return BranchResult{}, fmt.Errorf("branch %q already exists locally or on the remote (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) + } + 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 "" +} + +// CommitsAhead reports how many commits HEAD is ahead of the remote-tracking +// ref /. Auto-branching runs this before creating and pushing +// a feature branch off the default branch: a clean, up-to-date default branch +// has nothing to publish, so the caller can refuse rather than push an empty +// comparison. It returns an error when the count cannot be determined (for +// example the remote-tracking ref was never fetched); callers treat that as a +// hard failure rather than guessing that there is something to publish. +func CommitsAhead(ctx context.Context, cwd, remote, branch string, runGit Runner) (int, error) { + runGit, _ = resolveRunners(runGit, nil) + out, err := gitOutput(ctx, runGit, cwd, "rev-list", "--count", remote+"/"+branch+"..HEAD") + if err != nil { + return 0, err + } + count, err := strconv.Atoi(strings.TrimSpace(out)) + if err != nil { + return 0, fmt.Errorf("parse commit count %q: %w", out, err) + } + return count, nil +} + +// RefreshTrackingRef updates the local remote-tracking ref for / +// from the remote's current advertised tip. ensureFeatureBranch calls this +// before CommitsAhead: IsDefaultBranch already contacted the remote for its +// symref check, but a merely-cached origin/main (last written at clone or a +// previous fetch) can sit behind the remote's live tip, making a local-only +// rev-list comparison report nothing to publish when the remote has actually +// advanced. The explicit refspec updates the tracking ref regardless of the +// remote's configured fetch refspec. +func RefreshTrackingRef(ctx context.Context, cwd, remote, branch string, runGit Runner) error { + runGit, _ = resolveRunners(runGit, nil) + refspec := fmt.Sprintf("+refs/heads/%s:refs/remotes/%s/%s", branch, remote, branch) + // "--" terminates option parsing so a remote value shaped like an option + // (--upload-pack=/bin/echo) reaches Git as a positional argument. + _, err := gitOutput(ctx, runGit, cwd, "fetch", "--", remote, refspec) + return err +} + +// RemoteHasBranch reports whether remote already has refs/heads/. +// ensureFeatureBranch uses this when local upstream config is missing or +// wrong: a prior push may have published the branch even when push -u failed +// to write .git/config, and the nonexistence lease must not be reasserted +// against a ref that already exists on the remote. +func RemoteHasBranch(ctx context.Context, cwd, remote, branch string, runGit Runner) (bool, error) { + runGit, _ = resolveRunners(runGit, nil) + remote = strings.TrimSpace(remote) + branch = strings.TrimSpace(branch) + if remote == "" || branch == "" { + return false, nil + } + // Ask for the exact head ref rather than listing every branch. "--" + // terminates option parsing so a remote shaped like an option stays + // positional. + ref := "refs/heads/" + branch + out, err := gitOutput(ctx, runGit, cwd, "ls-remote", "--heads", "--", remote, ref) + if err != nil { + return false, err + } + for _, line := range strings.Split(out, "\n") { + line = strings.TrimSpace(line) + if line == "" { + continue + } + if _, got, ok := strings.Cut(line, "\t"); ok && strings.TrimSpace(got) == ref { + return true, nil + } + } + return false, nil +} + +// UpstreamRef returns the short remote-tracking name for branch's configured +// upstream (for example "origin/user/slug"), or "" when none is configured or +// the ref cannot be resolved. Callers that need to know whether Zero's own +// `push -u` published exactly / compare this string rather than +// reading branch..remote alone: with branch.autoSetupMerge=inherit, +// `git checkout -b` copies remote=origin and merge=refs/heads/main from the +// source branch before any push, so a remote config field is not publication +// state. +func UpstreamRef(ctx context.Context, cwd, branch string, runGit Runner) string { + runGit, _ = resolveRunners(runGit, nil) + branch = strings.TrimSpace(branch) + if branch == "" { + return "" + } + out, err := gitOutput(ctx, runGit, cwd, "rev-parse", "--abbrev-ref", branch+"@{upstream}") + if err != nil { + return "" + } + return strings.TrimSpace(out) +} + +// HasUpstream reports whether branch has a published upstream tracking the same +// branch name on a remote (the relationship `git push -u ` +// records). ensureFeatureBranch consults this when it is called again with a +// non-default current branch: a generated branch that just lost a +// force-with-lease race against a concurrent creator is left checked out +// locally without that relationship, so a retry must not treat it the same as +// an ordinary, already-published feature branch and drop the nonexistence +// lease. +// +// An inherited upstream to a different branch (branch.autoSetupMerge=inherit +// copies origin/main onto a new user/slug) is not publication state and reports +// false. Any failure to resolve the upstream (including "no upstream +// configured") also reports false so the caller keeps requiring the lease. +func HasUpstream(ctx context.Context, cwd, branch string, runGit Runner) (bool, error) { + branch = strings.TrimSpace(branch) + if branch == "" { + return false, nil + } + ref := UpstreamRef(ctx, cwd, branch, runGit) + if ref == "" { + return false, nil + } + // rev-parse --abbrev-ref prints "/"; branch names may + // themselves contain slashes (user/slug), so take everything after the + // first slash as the tracked branch name. + _, tracked, ok := strings.Cut(ref, "/") + if !ok || tracked == "" { + return false, nil + } + return tracked == branch, nil +} + +// UpstreamRemote returns the configured upstream remote name for branch (e.g. "origin"), +// or "" if no upstream is configured. This alone is not proof of publication: +// branch.autoSetupMerge=inherit copies branch..remote from the source +// branch before any push. Prefer UpstreamRef when checking that / +// was actually published. +func UpstreamRemote(ctx context.Context, cwd, branch string, runGit Runner) string { + runGit, _ = resolveRunners(runGit, nil) + out, err := gitOutput(ctx, runGit, cwd, "config", "branch."+branch+".remote") + if err != nil { + return "" + } + return strings.TrimSpace(out) +} + +// DeleteBranch switches to fallbackBranch and deletes branchToDelete. +func DeleteBranch(ctx context.Context, cwd, fallbackBranch, branchToDelete string, runGit Runner) error { + runGit, _ = resolveRunners(runGit, nil) + if _, err := gitOutput(ctx, runGit, cwd, "checkout", fallbackBranch); err != nil { + return err + } + _, err := gitOutput(ctx, runGit, cwd, "branch", "-D", branchToDelete) + return err +} + +// CurrentBranch returns the short name of the currently checked-out branch, or +// "" when HEAD is detached / unresolvable. Callers that need the branch's +// configured upstream (for example the --yes unborn-remote preflight) use this +// instead of IsDefaultBranch so a remote-HEAD lookup failure does not block +// remote resolution. +func CurrentBranch(ctx context.Context, cwd string, runGit Runner) string { + runGit, _ = resolveRunners(runGit, nil) + out, err := gitOutput(ctx, runGit, cwd, "rev-parse", "--abbrev-ref", "HEAD") + if err != nil { + return "" + } + branch := strings.TrimSpace(out) + if branch == "" || branch == "HEAD" { + return "" + } + return branch +} + +// ResetBranchRef points the local branch ref at newTip without checking the +// branch out. ensureFeatureBranch uses this after creating a feature branch +// that owns the publishable commits: the normal flow commits on the default +// branch first, then creates user/slug at the same HEAD, and without this +// restore the local default keeps those commits and diverges from the remote +// after a squash-merge. newTip is typically the remote-tracking ref +// (origin/main) that CommitsAhead already used. Refuses to move the currently +// checked-out branch so callers must have already switched to the feature +// branch. +func ResetBranchRef(ctx context.Context, cwd, branch, newTip string, runGit Runner) error { + runGit, _ = resolveRunners(runGit, nil) + branch = strings.TrimSpace(branch) + newTip = strings.TrimSpace(newTip) + if branch == "" { + return fmt.Errorf("branch name required") + } + if newTip == "" { + return fmt.Errorf("new tip required") + } + // Branch becomes refs/heads/; reject traversal / absolute forms so + // a hostile branch name cannot escape the heads namespace. + if strings.Contains(branch, "..") || strings.HasPrefix(branch, "/") || strings.ContainsAny(branch, "\\ \t\n") { + return fmt.Errorf("invalid branch name %q", branch) + } + headBranch, err := gitOutput(ctx, runGit, cwd, "rev-parse", "--abbrev-ref", "HEAD") + if err == nil && strings.TrimSpace(headBranch) == branch { + return fmt.Errorf("refusing to move currently checked-out branch %q", branch) + } + tipSHA, err := gitOutput(ctx, runGit, cwd, "rev-parse", "--verify", newTip+"^{commit}") + if err != nil { + return fmt.Errorf("resolve tip %q: %w", newTip, err) + } + if _, err := gitOutput(ctx, runGit, cwd, "update-ref", "refs/heads/"+branch, strings.TrimSpace(tipSHA)); err != nil { + return fmt.Errorf("update-ref refs/heads/%s: %w", branch, err) + } + return nil +} + +// IsUnbornRemote reports whether remote is a freshly created repository with +// no refs at all (no branches, no HEAD). ensureFeatureBranch consults this +// when CommitsAhead fails to determine why: a genuinely empty remote has no +// / tracking ref for CommitsAhead to diff against, and that +// is proof there is nothing published yet, not an unknown state to fail +// closed on. An error here (unreachable remote, timeout) leaves the state +// unconfirmed, so callers must treat that the same as a non-empty remote. +func IsUnbornRemote(ctx context.Context, cwd, remote string, runGit Runner) (bool, error) { + runGit, _ = resolveRunners(runGit, nil) + // "--" terminates option parsing so a remote value shaped like an option + // (--upload-pack=/bin/echo) reaches Git as a positional argument. + out, err := gitOutput(ctx, runGit, cwd, "ls-remote", "--heads", "--", remote) + if err != nil { + return false, err + } + return strings.TrimSpace(out) == "", 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 { @@ -682,3 +1196,18 @@ func CreatePR(ctx context.Context, options PROptions) (PRResult, error) { Output: res.Stdout, }, nil } + +// MarkGeneratedBranch records local branch configuration marking branch as +// auto-generated by Zero. +func MarkGeneratedBranch(ctx context.Context, cwd, branch string, runGit Runner) error { + runGit, _ = resolveRunners(runGit, nil) + _, err := gitOutput(ctx, runGit, cwd, "config", "branch."+branch+".zeroAutoBranch", "true") + return err +} + +// IsGeneratedBranch reports whether branch was marked as auto-generated by Zero. +func IsGeneratedBranch(ctx context.Context, cwd, branch string, runGit Runner) bool { + runGit, _ = resolveRunners(runGit, nil) + out, err := gitOutput(ctx, runGit, cwd, "config", "--get", "branch."+branch+".zeroAutoBranch") + return err == nil && strings.TrimSpace(out) == "true" +} diff --git a/internal/zerogit/zerogit_test.go b/internal/zerogit/zerogit_test.go index cfb479e6e..055e4ab38 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" @@ -529,9 +530,10 @@ 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 - {}, // ls-remote --symref (no match → falls through) + {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"}, + {Stdout: "origin/feat/some-feature\n"}, // UpstreamRef after push -u }} result, err := Push(context.Background(), PushOptions{ @@ -556,9 +558,10 @@ 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 - {}, // ls-remote --symref (no match) + {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"}, + {Stdout: "origin/feat/some-feature\n"}, // UpstreamRef after push -u }} _, err := Push(context.Background(), PushOptions{ @@ -596,11 +599,18 @@ func TestPushBranchesToRemote(t *testing.T) { }) t.Run("RejectsDefaultBranch", func(t *testing.T) { + // The conventional main/master name is only a fallback for when the + // remote's actual default cannot be determined live or from the local + // cache, so the remote is consulted (and found unreachable, with no + // cached record either) before the name heuristic applies. for _, branch := range []string{"main", "master"} { root := t.TempDir() runner := &fakeRunner{results: []CommandResult{ {Stdout: root + "\n"}, {Stdout: branch + "\n"}, + {ExitCode: 1}, // config branch..remote unset → origin + {ExitCode: 128, Stderr: "fatal:"}, // ls-remote fails + {ExitCode: 1}, // no local refs/remotes/origin/HEAD record }} _, err := Push(context.Background(), PushOptions{ @@ -623,6 +633,7 @@ func TestPushBranchesToRemote(t *testing.T) { {Stdout: "main\n"}, {Stdout: "origin\n"}, {Stdout: "Everything up-to-date\n"}, + {Stdout: "origin/main\n"}, // UpstreamRef after push -u }} result, err := Push(context.Background(), PushOptions{ @@ -643,9 +654,10 @@ 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 - {}, // ls-remote --symref (no match) + {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"}, + {Stdout: "origin/feat/some-feature\n"}, // UpstreamRef after push -u }} result, err := Push(context.Background(), PushOptions{ @@ -664,6 +676,117 @@ 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) + } + }) + + t.Run("RequireNewRemoteBranchGuardsAgainstConcurrentCreation", func(t *testing.T) { + // CreateBranch's own remote-collision probe runs before this push, so + // a concurrent creator of the same name in that window would + // otherwise be silently fast-forwarded. RequireNewRemoteBranch closes + // it with a zero-value --force-with-lease asserting the destination + // still doesn't exist at push time. + root := t.TempDir() + runner := &fakeRunner{results: []CommandResult{ + {Stdout: root + "\n"}, + {Stdout: "alice/fix-typo\n"}, + {Stdout: "origin\n"}, // config branch.alice/fix-typo.remote + {Stdout: "ref: refs/heads/main\tHEAD\nabc123\tHEAD\n"}, // ls-remote --symref: default is main + {Stdout: "Everything up-to-date\n"}, + {Stdout: "origin/alice/fix-typo\n"}, // UpstreamRef after push -u + }} + + _, err := Push(context.Background(), PushOptions{ + Cwd: root, + RunGit: runner.Run, + RequireNewRemoteBranch: true, + }) + if err != nil { + t.Fatalf("Push returned error: %v", err) + } + if got := runner.commandLine(4); got != "git push --force-with-lease=alice/fix-typo: -u -- origin alice/fix-typo" { + t.Fatalf("unexpected push command: %q", got) + } + }) + + t.Run("RecoversWhenPushUCannotWriteLocalUpstream", func(t *testing.T) { + // git push -u can exit 0 after publishing the remote branch while + // failing to write .git/config (config.lock). Push must recover via + // branch --set-upstream-to rather than reporting a full success with + // no local upstream. + root := t.TempDir() + runner := &fakeRunner{results: []CommandResult{ + {Stdout: root + "\n"}, + {Stdout: "user/slug\n"}, + {Stdout: "origin\n"}, + {Stdout: "ref: refs/heads/main\tHEAD\nabc123\tHEAD\n"}, + {Stdout: "To origin\n * [new branch] user/slug -> user/slug\n"}, + {ExitCode: 128, Stderr: "fatal: no upstream configured for branch 'user/slug'"}, // UpstreamRef missing + {Stdout: ""}, // branch --set-upstream-to=origin/user/slug user/slug + {Stdout: "origin/user/slug\n"}, // UpstreamRef after recovery + }} + + result, err := Push(context.Background(), PushOptions{ + Cwd: root, + RunGit: runner.Run, + }) + if err != nil { + t.Fatalf("Push returned error: %v", err) + } + if result.Remote != "origin" || result.Branch != "user/slug" { + t.Fatalf("unexpected push result: %#v", result) + } + if got := runner.commandLine(6); got != "git branch --set-upstream-to=origin/user/slug user/slug" { + t.Fatalf("expected set-upstream recovery, got %q", got) + } + }) + + t.Run("SurfacesUpstreamWriteFailureWhenRecoveryFails", func(t *testing.T) { + root := t.TempDir() + runner := &fakeRunner{results: []CommandResult{ + {Stdout: root + "\n"}, + {Stdout: "user/slug\n"}, + {Stdout: "origin\n"}, + {Stdout: "ref: refs/heads/main\tHEAD\nabc123\tHEAD\n"}, + {Stdout: "To origin\n * [new branch] user/slug -> user/slug\n"}, + {ExitCode: 128, Stderr: "fatal: no upstream configured"}, + {ExitCode: 255, Stderr: "error: could not write config file .git/config: File exists"}, + }} + + result, err := Push(context.Background(), PushOptions{ + Cwd: root, + RunGit: runner.Run, + }) + if err == nil { + t.Fatal("expected upstream-write failure after successful remote publish") + } + if !strings.Contains(err.Error(), "push published origin/user/slug") || !strings.Contains(err.Error(), "failed to configure local upstream") { + t.Fatalf("unexpected error: %v", err) + } + // Remote side did publish: result still carries remote/branch for callers. + if result.Remote != "origin" || result.Branch != "user/slug" { + t.Fatalf("expected partial result for published branch, got %#v", result) + } + }) } func TestCreatePRCommandConstruction(t *testing.T) { @@ -727,3 +850,730 @@ 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"}, + {ExitCode: 1}, // rev-parse --verify: no local branch by that name yet + {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 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("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: 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{ + 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-2" { + t.Fatalf("unexpected branch: %#v", result) + } + 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) + } + }) + + 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{ + {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("ResolvesCurrentBranchByConventionalName", func(t *testing.T) { + // The live symref confirms main is genuinely the remote's default here; + // the conventional-name fallback below only applies when the remote + // can't answer at all (see FallbackToConventionalNameWhenRemoteUnknown). + root := t.TempDir() + runner := &fakeRunner{results: []CommandResult{ + {Stdout: root + "\n"}, + {Stdout: "main\n"}, + {ExitCode: 1}, // config branch.main.remote unset → origin + {Stdout: "ref: refs/heads/main\tHEAD\nabc123\tHEAD\n"}, // ls-remote --symref: default is main + }} + + 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" || remote != "origin" { + t.Fatalf("unexpected result: isDefault=%v branch=%q remote=%q", isDefault, branch, remote) + } + }) + + t.Run("FallbackToConventionalNameWhenRemoteUnknown", func(t *testing.T) { + // The conventional main/master name only decides the result when the + // remote's actual default genuinely cannot be determined, live or + // cached. + root := t.TempDir() + runner := &fakeRunner{results: []CommandResult{ + {Stdout: root + "\n"}, + {Stdout: "main\n"}, + {ExitCode: 1}, // config branch.main.remote unset → origin + {ExitCode: 128, Stderr: "fatal:"}, // ls-remote fails + {ExitCode: 1}, // no local refs/remotes/origin/HEAD record + }} + + 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" || remote != "origin" { + t.Fatalf("unexpected result: isDefault=%v branch=%q remote=%q", isDefault, branch, remote) + } + }) + + // This is the regression test for jatmn's P2 finding: resolve main/master + // against the selected remote before treating it as protected. A + // repository whose remote default is genuinely "trunk" can have a + // legitimate non-default local "main" (e.g. tracking origin/trunk); the + // live symref result must win over the conventional-name fallback. + t.Run("NonDefaultLocalMainTrackingADifferentRemoteDefault", func(t *testing.T) { + root := t.TempDir() + runner := &fakeRunner{results: []CommandResult{ + {Stdout: root + "\n"}, + {Stdout: "main\n"}, + {Stdout: "origin\n"}, // config branch.main.remote + {Stdout: "ref: refs/heads/trunk\tHEAD\nabc123\tHEAD\n"}, // ls-remote --symref: default is trunk + }} + + 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 { + t.Fatal("local main tracking a remote whose live default is trunk must not be treated as protected") + } + if branch != "main" || remote != "origin" { + t.Fatalf("unexpected result: branch=%q remote=%q", branch, remote) + } + }) + + 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"}, + {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, remote, 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" || 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) + } + }) + + t.Run("StaleCachedRemoteHeadDoesNotClearGuard", func(t *testing.T) { + // The server renamed its default from main to trunk, but the local + // refs/remotes/origin/HEAD cache still names main. With the live lookup + // failing, a cache that does not match the branch is not evidence the + // branch is unprotected: the check must fail closed, not report "trunk + // is not the default" and let the push through without --yes. + 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/main\n"}, // stale cache: still says main + }} + + _, _, _, 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 on stale cache mismatch, got %v", err) + } + }) +} + +func TestCommitsAhead(t *testing.T) { + t.Run("CountsCommitsAheadOfRemoteDefault", func(t *testing.T) { + root := t.TempDir() + runner := &fakeRunner{results: []CommandResult{ + {Stdout: "3\n"}, + }} + count, err := CommitsAhead(context.Background(), root, "origin", "main", runner.Run) + if err != nil { + t.Fatalf("CommitsAhead returned error: %v", err) + } + if count != 3 { + t.Fatalf("count = %d, want 3", count) + } + if got := runner.commandLine(0); got != "git rev-list --count origin/main..HEAD" { + t.Fatalf("unexpected command: %q", got) + } + }) + + t.Run("ReturnsErrorWhenRemoteTrackingRefMissing", func(t *testing.T) { + // A never-fetched remote-tracking ref makes rev-list fail; the caller + // treats that as "cannot tell" and proceeds rather than block a push. + root := t.TempDir() + runner := &fakeRunner{results: []CommandResult{ + {ExitCode: 128, Stderr: "fatal: ambiguous argument 'origin/main..HEAD'"}, + }} + if _, err := CommitsAhead(context.Background(), root, "origin", "main", runner.Run); err == nil { + t.Fatal("expected an error when the remote-tracking ref is missing") + } + }) +} + +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) + } +} + +// 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", + " 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") + } +} + +func TestIsDefaultBranchTerminatesOptionsBeforeRemote(t *testing.T) { + // A remote value that looks like a Git option (from --remote or branch + // config) must reach ls-remote as a positional argument after "--", + // never be parsed as an option such as --upload-pack. + root := t.TempDir() + runner := &fakeRunner{results: []CommandResult{ + {Stdout: root + "\n"}, + {Stdout: "ref: refs/heads/main\tHEAD\nabc123\tHEAD\n"}, + }} + + _, _, _, err := IsDefaultBranch(context.Background(), DefaultBranchOptions{ + Cwd: root, + Branch: "feature", + Remote: "--upload-pack=/bin/echo", + RunGit: runner.Run, + }) + if err != nil { + t.Fatalf("IsDefaultBranch returned error: %v", err) + } + if got := runner.commandLine(1); got != "git ls-remote --symref -- --upload-pack=/bin/echo HEAD" { + t.Fatalf("remote was not terminated with --: %q", got) + } +} + +func TestIsDefaultBranchAllowsFirstPushToUnbornRemote(t *testing.T) { + // A freshly created empty remote has no refs, so ls-remote succeeds with + // empty output and `git remote set-head --auto` cannot record a default. + // The guard must not turn the very first feature-branch push into a + // --yes dead end; main/master stay protected by the name heuristic. The + // empty symref output must still be confirmed against `ls-remote --heads` + // before granting the exception (see the dangling-HEAD test below), so a + // genuinely unborn remote answers empty there too. + root := t.TempDir() + runner := &fakeRunner{results: []CommandResult{ + {Stdout: root + "\n"}, + {Stdout: "\n"}, // ls-remote --symref: remote answered, zero refs + {Stdout: "\n"}, // ls-remote --heads: confirms no branches at all + }} + + isDefault, _, _, err := IsDefaultBranch(context.Background(), DefaultBranchOptions{ + Cwd: root, + Branch: "alice/first-work", + Remote: "origin", + RunGit: runner.Run, + }) + if err != nil { + t.Fatalf("IsDefaultBranch on an unborn remote: %v", err) + } + if isDefault { + t.Fatal("feature branch on an unborn remote reported as default") + } +} + +func TestIsDefaultBranchClassifiesConventionalDefaultOnUnbornRemote(t *testing.T) { + root := t.TempDir() + runner := &fakeRunner{results: []CommandResult{ + {Stdout: root + "\n"}, + {Stdout: "\n"}, // ls-remote --symref: remote answered, zero refs + {Stdout: "\n"}, // ls-remote --heads: confirms no branches at all + }} + + isDefault, _, _, err := IsDefaultBranch(context.Background(), DefaultBranchOptions{ + Cwd: root, + Branch: "main", + Remote: "origin", + RunGit: runner.Run, + }) + if err != nil { + t.Fatalf("IsDefaultBranch on an unborn remote: %v", err) + } + if !isDefault { + t.Fatal("conventional main branch on an unborn remote should be reported as default") + } +} + +func TestIsDefaultBranchFailsClosedOnDanglingRemoteHead(t *testing.T) { + // A non-empty remote whose HEAD symref is dangling or missing produces + // the exact same empty `ls-remote --symref` output as a genuinely unborn + // remote, but it may still have a protected default branch under a name + // this can't identify. `ls-remote --heads` reporting existing branches + // must block the unborn-repository exception and fail closed rather than + // silently treat the branch as safe to push. + root := t.TempDir() + runner := &fakeRunner{results: []CommandResult{ + {Stdout: root + "\n"}, + {Stdout: "\n"}, // ls-remote --symref: HEAD didn't resolve + {Stdout: "abc123\trefs/heads/trunk\n"}, // ls-remote --heads: remote is NOT empty + {ExitCode: 1}, // no local refs/remotes/origin/HEAD record + }} + + _, _, _, err := IsDefaultBranch(context.Background(), DefaultBranchOptions{ + Cwd: root, + Branch: "alice/first-work", + Remote: "origin", + RunGit: runner.Run, + }) + if err == nil || !strings.Contains(err.Error(), "default branch for remote") { + t.Fatalf("expected fail-closed error on dangling remote HEAD, got %v", err) + } +} + +func TestCreateBranchAvoidsRemoteOnlyCollision(t *testing.T) { + // A branch that exists only on the target remote (an old merged-PR + // branch pruned locally) must count as taken: `push -u` would otherwise + // silently fast-forward it with unrelated new work. + root := t.TempDir() + runner := &fakeRunner{results: []CommandResult{ + {Stdout: root + "\n"}, + {Stdout: "abc123\trefs/heads/alice/fix-typo\nqrs789\trefs/heads/other\n"}, // ls-remote --heads + {ExitCode: 1}, // rev-parse: alice/fix-typo not local, but remote-taken + {ExitCode: 1}, // rev-parse: alice/fix-typo-2 free locally + {Stdout: "Switched to a new branch 'alice/fix-typo-2'\n"}, + }} + + result, err := CreateBranch(context.Background(), BranchOptions{ + Cwd: root, + Name: "alice/fix-typo", + Remote: "origin", + RunGit: runner.Run, + }) + if err != nil { + t.Fatalf("CreateBranch returned error: %v", err) + } + if result.Branch != "alice/fix-typo-2" { + t.Fatalf("unexpected branch: %#v", result) + } + if got := runner.commandLine(1); got != "git ls-remote --heads -- origin" { + t.Fatalf("unexpected remote probe: %q", got) + } +} + +func TestCreateBranchFailsWhenRemoteProbeFails(t *testing.T) { + // When the target remote cannot be consulted, fail visibly instead of + // risking a push onto an unseen remote-only branch; the push itself + // would need the same connectivity anyway. + root := t.TempDir() + runner := &fakeRunner{results: []CommandResult{ + {Stdout: root + "\n"}, + {ExitCode: 128, Stderr: "fatal: unable to access"}, // ls-remote --heads fails + }} + + _, err := CreateBranch(context.Background(), BranchOptions{ + Cwd: root, + Name: "alice/fix-typo", + Remote: "origin", + RunGit: runner.Run, + }) + if err == nil { + t.Fatal("expected an error when the remote probe fails") + } +} + +// TestResetBranchRefMovesDefaultWithoutTouchingFeature covers the post +// auto-branch restore: after checkout -b user/slug at the same HEAD as main, +// main must be moved back to origin/main so the feature branch exclusively +// owns the publishable commits. +func TestResetBranchRefMovesDefaultWithoutTouchingFeature(t *testing.T) { + root := initGitRepo(t, true) + // Normalize the default branch name (git may pick master on older installs). + runGitCommand(t, root, "branch", "-M", "main") + base := strings.TrimSpace(runGitCommand(t, root, "rev-parse", "HEAD")) + + // Simulate origin/main at the base tip, then a local commit on main. + runGitCommand(t, root, "update-ref", "refs/remotes/origin/main", base) + writeTestFile(t, filepath.Join(root, "feature.txt"), "work\n") + runGitCommand(t, root, "add", "feature.txt") + runGitCommand(t, root, "-c", "user.name=Zero", "-c", "user.email=zero@example.invalid", "commit", "-m", "add feature") + featureTip := strings.TrimSpace(runGitCommand(t, root, "rev-parse", "HEAD")) + if featureTip == base { + t.Fatal("expected a new commit on main before branching") + } + + // Create the feature branch at HEAD (same as ensureFeatureBranch / CreateBranch). + runGitCommand(t, root, "checkout", "-b", "someone/feature-txt") + if err := ResetBranchRef(context.Background(), root, "main", "origin/main", nil); err != nil { + t.Fatalf("ResetBranchRef: %v", err) + } + + mainTip := strings.TrimSpace(runGitCommand(t, root, "rev-parse", "refs/heads/main")) + if mainTip != base { + t.Fatalf("main tip = %s, want base %s (must not keep the new commit)", mainTip, base) + } + stillFeature := strings.TrimSpace(runGitCommand(t, root, "rev-parse", "refs/heads/someone/feature-txt")) + if stillFeature != featureTip { + t.Fatalf("feature branch tip = %s, want %s", stillFeature, featureTip) + } + head := strings.TrimSpace(runGitCommand(t, root, "rev-parse", "--abbrev-ref", "HEAD")) + if head != "someone/feature-txt" { + t.Fatalf("HEAD = %q, want someone/feature-txt", head) + } +} + +func TestResetBranchRefRefusesCheckedOutBranch(t *testing.T) { + root := initGitRepo(t, true) + runGitCommand(t, root, "branch", "-M", "main") + base := strings.TrimSpace(runGitCommand(t, root, "rev-parse", "HEAD")) + runGitCommand(t, root, "update-ref", "refs/remotes/origin/main", base) + + err := ResetBranchRef(context.Background(), root, "main", "origin/main", nil) + if err == nil || !strings.Contains(err.Error(), "currently checked-out") { + t.Fatalf("expected refuse-checked-out error, got %v", err) + } +} + +func TestCurrentBranchReturnsCheckedOutName(t *testing.T) { + root := initGitRepo(t, true) + runGitCommand(t, root, "branch", "-M", "main") + if got := CurrentBranch(context.Background(), root, nil); got != "main" { + t.Fatalf("CurrentBranch = %q, want main", got) + } + runGitCommand(t, root, "checkout", "-b", "feature/work") + if got := CurrentBranch(context.Background(), root, nil); got != "feature/work" { + t.Fatalf("CurrentBranch = %q, want feature/work", got) + } +} + +// TestRemoteHasBranchSeesPushWithoutLocalUpstream is the publication side of +// the push -u config-write race: a plain `git push` (no -u) leaves local +// upstream empty while the remote branch exists. ensureFeatureBranch must +// probe this rather than reasserting the nonexistence lease. +func TestRemoteHasBranchSeesPushWithoutLocalUpstream(t *testing.T) { + if _, err := exec.LookPath("git"); err != nil { + t.Skipf("git unavailable: %v", err) + } + tmp := t.TempDir() + bare := filepath.Join(tmp, "remote.git") + repo := filepath.Join(tmp, "repo") + runGitCommand(t, tmp, "init", "--bare", bare) + runGitCommand(t, tmp, "init", repo) + runGitCommand(t, repo, "config", "user.name", "Zero") + runGitCommand(t, repo, "config", "user.email", "zero@example.invalid") + runGitCommand(t, repo, "checkout", "-b", "main") + writeTestFile(t, filepath.Join(repo, "README.md"), "initial\n") + runGitCommand(t, repo, "add", "README.md") + runGitCommand(t, repo, "commit", "-m", "Initial commit") + runGitCommand(t, repo, "remote", "add", "origin", bare) + runGitCommand(t, repo, "push", "-u", "origin", "main") + runGitCommand(t, repo, "checkout", "-b", "user/slug") + writeTestFile(t, filepath.Join(repo, "README.md"), "feature work\n") + runGitCommand(t, repo, "add", "README.md") + runGitCommand(t, repo, "commit", "-m", "feature") + // Publish without -u so local upstream stays unset (same observable state + // as push -u succeeding on the remote then failing to write .git/config). + runGitCommand(t, repo, "push", "origin", "user/slug") + + if ref := UpstreamRef(context.Background(), repo, "user/slug", nil); ref != "" { + t.Fatalf("UpstreamRef after push without -u = %q, want empty", ref) + } + exists, err := RemoteHasBranch(context.Background(), repo, "origin", "user/slug", nil) + if err != nil { + t.Fatalf("RemoteHasBranch: %v", err) + } + if !exists { + t.Fatal("RemoteHasBranch must report the published branch even without local upstream") + } + missing, err := RemoteHasBranch(context.Background(), repo, "origin", "user/other", nil) + if err != nil { + t.Fatalf("RemoteHasBranch missing: %v", err) + } + if missing { + t.Fatal("RemoteHasBranch must be false for an unpublished name") + } +} + +// TestHasUpstreamRejectsInheritedMainUpstream covers branch.autoSetupMerge=inherit: +// checkout -b copies origin/main onto the new branch before any push -u. That +// must not count as a published upstream for the generated branch name. +func TestHasUpstreamRejectsInheritedMainUpstream(t *testing.T) { + if _, err := exec.LookPath("git"); err != nil { + t.Skipf("git unavailable: %v", err) + } + tmp := t.TempDir() + bare := filepath.Join(tmp, "remote.git") + repo := filepath.Join(tmp, "repo") + runGitCommand(t, tmp, "init", "--bare", bare) + runGitCommand(t, tmp, "init", repo) + runGitCommand(t, repo, "config", "user.name", "Zero") + runGitCommand(t, repo, "config", "user.email", "zero@example.invalid") + runGitCommand(t, repo, "checkout", "-b", "main") + writeTestFile(t, filepath.Join(repo, "README.md"), "initial\n") + runGitCommand(t, repo, "add", "README.md") + runGitCommand(t, repo, "commit", "-m", "Initial commit") + runGitCommand(t, repo, "remote", "add", "origin", bare) + runGitCommand(t, repo, "push", "-u", "origin", "main") + runGitCommand(t, repo, "config", "branch.autoSetupMerge", "inherit") + runGitCommand(t, repo, "checkout", "-b", "user/slug") + + if ref := UpstreamRef(context.Background(), repo, "user/slug", nil); ref != "origin/main" { + t.Fatalf("UpstreamRef after inherit = %q, want origin/main", ref) + } + has, err := HasUpstream(context.Background(), repo, "user/slug", nil) + if err != nil { + t.Fatalf("HasUpstream: %v", err) + } + if has { + t.Fatal("HasUpstream must reject inherited origin/main for user/slug") + } + + runGitCommand(t, repo, "push", "-u", "origin", "user/slug") + if ref := UpstreamRef(context.Background(), repo, "user/slug", nil); ref != "origin/user/slug" { + t.Fatalf("UpstreamRef after push -u = %q, want origin/user/slug", ref) + } + has, err = HasUpstream(context.Background(), repo, "user/slug", nil) + if err != nil { + t.Fatalf("HasUpstream after push: %v", err) + } + if !has { + t.Fatal("HasUpstream must accept exact origin/user/slug after push -u") + } +}