refactor(git): unify state-push rebase-retry into one dir-aware helper

internal/git/git.go

@@ -199,46 +199,125 @@ func ListTags() ([]string, error) {
 	return parseLines(output), nil
 }
 
+// pushRetryAttempts is the number of times a rejected push is retried behind a
+// rebase before the operation is declared failed.
+const pushRetryAttempts = 3
+
+// defaultPushBackoff is the delay between push retries when no backoff is set via
+// WithBackoff. It gives a concurrent state writer time to settle before the next
+// attempt.
+const defaultPushBackoff = 2 * time.Second
+
+// pushOptions holds the tunable behaviour of the rebase-retry push helpers. Its
+// zero value reproduces the historical behaviour: git runs in the process working
+// directory and retries wait defaultPushBackoff apart.
+type pushOptions struct {
+	dir     string
+	backoff time.Duration
+}
+
+// Option configures the rebase-retry push helpers. Options are additive: a call
+// with no options behaves exactly as the original positional API did.
+type Option func(*pushOptions)
+
+// WithDir runs the git commands with cmd.Dir set to dir instead of the process
+// working directory. An empty dir (the default) leaves the process working
+// directory in effect. This lets a caller drive a repository other than the one
+// the process was launched in.
+func WithDir(dir string) Option {
+	return func(o *pushOptions) { o.dir = dir }
+}
+
+// WithBackoff sets the delay between push retries. A zero duration (the default)
+// selects defaultPushBackoff. Tests pass a tiny value to keep the retry loop fast.
+func WithBackoff(d time.Duration) Option {
+	return func(o *pushOptions) { o.backoff = d }
+}
+
+func newPushOptions(opts []Option) pushOptions {
+	var o pushOptions
+	for _, opt := range opts {
+		opt(&o)
+	}
+	if o.backoff == 0 {
+		o.backoff = defaultPushBackoff
+	}
+	return o
+}
+
 // CommitAndPushWithRetry stages filePath, commits it with message, and pushes
 // to the current branch's upstream, retrying the push up to three times behind a
 // pull --rebase. A "nothing to commit" state is treated as success (no-op). This
 // is the manifest state-write path shared by promote and hotfix finalize: an
 // API-created commit on real GitHub goes through a different path, so this is the
 // plain-git fallback used when committing locally.
-func CommitAndPushWithRetry(filePath, message string) error {
+//
+// Optional behaviour is supplied through Options: WithDir runs the commands in a
+// specific repository, and WithBackoff tunes the retry delay. With no options the
+// call behaves identically to the original positional signature.
+func CommitAndPushWithRetry(filePath, message string, opts ...Option) error {
+	o := newPushOptions(opts)
+
 	cmd := exec.Command("git", "add", filePath)
+	cmd.Dir = o.dir
 	if out, err := cmd.CombinedOutput(); err != nil {
 		return fmt.Errorf("git add failed: %s: %w", string(out), err)
 	}
 
 	cmd = exec.Command("git", "commit", "-m", message)
+	cmd.Dir = o.dir
 	if out, err := cmd.CombinedOutput(); err != nil {
 		if strings.Contains(string(out), "nothing to commit") {
 			return nil
 		}
 		return fmt.Errorf("git commit failed: %s: %w", string(out), err)
 	}
 
-	for i := 0; i < 3; i++ {
-		cmd = exec.Command("git", "push")
+	return pushWithRebaseRetry(o)
+}
+
+// PushWithRebaseRetry pushes the current branch to its upstream, retrying up to
+// three times behind a "git pull --rebase" when the push is rejected (for example
+// a non-fast-forward caused by a concurrent state writer landing on trunk between
+// checkout and push). It is the push half of CommitAndPushWithRetry, exposed for
+// callers that stage and commit through their own flow and only need the shared
+// rebase-retry behaviour. On a rebase conflict it aborts the rebase and returns
+// the wrapped error rather than leaving the repository mid-rebase.
+func PushWithRebaseRetry(opts ...Option) error {
+	return pushWithRebaseRetry(newPushOptions(opts))
+}
+
+// pushWithRebaseRetry is the single rebase-retry loop shared by
+// CommitAndPushWithRetry and PushWithRebaseRetry. Keeping one implementation
+// means the rebase-abort-on-conflict fix lives in exactly one place.
+func pushWithRebaseRetry(o pushOptions) error {
+	for i := 0; i < pushRetryAttempts; i++ {
+		cmd := exec.Command("git", "push")
+		cmd.Dir = o.dir
 		if _, err := cmd.CombinedOutput(); err == nil {
 			return nil
 		}
 
+		if i == pushRetryAttempts-1 {
+			break
+		}
+
 		cmd = exec.Command("git", "pull", "--rebase")
+		cmd.Dir = o.dir
 		if out, err := cmd.CombinedOutput(); err != nil {
 			// A failed rebase (typically a conflict) leaves the repository
 			// mid-rebase. Abort it so we neither leave a conflicted state
 			// behind nor loop into a guaranteed-failing push, and surface the
 			// real error instead of the generic "push failed" summary.
 			abort := exec.Command("git", "rebase", "--abort")
+			abort.Dir = o.dir
 			_, _ = abort.CombinedOutput() // best effort; nothing to abort is fine
 			return fmt.Errorf("git pull --rebase failed: %s: %w", string(out), err)
 		}
-		time.Sleep(2 * time.Second)
+		time.Sleep(o.backoff)
 	}
 
-	return fmt.Errorf("git push failed after 3 retries")
+	return fmt.Errorf("git push failed after %d retries", pushRetryAttempts)
 }
 
 // CurrentBranch returns the name of the currently checked-out branch.

internal/git/git_test.go

@@ -7,6 +7,7 @@ import (
 	"reflect"
 	"strings"
 	"testing"
+	"time"
 )
 
 // newScratchRepo initializes a git repository in a temp directory, changes the
@@ -438,6 +439,106 @@ func TestCommitAndPushWithRetry_AbortsRebaseOnConflict(t *testing.T) {
 	}
 }
 
+// sharedRemoteClones builds a bare remote plus two working clones tracking it.
+// The seed clone commits and pushes seedFile; the other clone then advances the
+// remote with otherFile so a subsequent push from seed is rejected as
+// non-fast-forward. When otherFile equals seedFile the advance conflicts on the
+// same path, setting up a genuine rebase conflict. Both clones have signing
+// disabled and an identity configured.
+func sharedRemoteClones(t *testing.T, seedFile, seedBody, otherFile, otherBody string) (seed, other string) {
+	t.Helper()
+
+	origin := t.TempDir()
+	if out, err := exec.Command("git", "init", "--bare", "--initial-branch=main", origin).CombinedOutput(); err != nil {
+		t.Fatalf("git init --bare: %v\n%s", err, out)
+	}
+
+	seed = t.TempDir()
+	gitAt(t, "", "clone", origin, seed)
+	configRepo(t, seed)
+	if err := os.WriteFile(filepath.Join(seed, seedFile), []byte(seedBody), 0o600); err != nil {
+		t.Fatalf("write seed file: %v", err)
+	}
+	gitAt(t, seed, "add", seedFile)
+	gitAt(t, seed, "commit", "-m", "seed state")
+	gitAt(t, seed, "push", "origin", "main")
+
+	other = t.TempDir()
+	gitAt(t, "", "clone", origin, other)
+	configRepo(t, other)
+	if err := os.WriteFile(filepath.Join(other, otherFile), []byte(otherBody), 0o600); err != nil {
+		t.Fatalf("write other file: %v", err)
+	}
+	gitAt(t, other, "add", otherFile)
+	gitAt(t, other, "commit", "-m", "concurrent write")
+	gitAt(t, other, "push", "origin", "main")
+
+	return seed, other
+}
+
+// TestPushWithRebaseRetry_RetriesNonFastForward proves the exported push half of
+// the shared helper rebases onto an advanced upstream and retries when the first
+// push is rejected non-fast-forward. WithDir drives the seed clone without
+// changing the process working directory, and WithBackoff keeps the retry fast.
+func TestPushWithRebaseRetry_RetriesNonFastForward(t *testing.T) {
+	// The concurrent writer touches an unrelated file so the rebase replays
+	// cleanly rather than conflicting.
+	seed, _ := sharedRemoteClones(t, "state.txt", "base\n", "OTHER.md", "concurrent\n")
+
+	// A local, committed change on the seed clone whose base is now behind trunk.
+	if err := os.WriteFile(filepath.Join(seed, "state.txt"), []byte("local change\n"), 0o600); err != nil {
+		t.Fatalf("write local file: %v", err)
+	}
+	gitAt(t, seed, "add", "state.txt")
+	gitAt(t, seed, "commit", "-m", "local change")
+
+	if err := PushWithRebaseRetry(WithDir(seed), WithBackoff(time.Millisecond)); err != nil {
+		t.Fatalf("PushWithRebaseRetry should rebase and retry a non-fast-forward push, got: %v", err)
+	}
+
+	log := runGitOut(t, seed, "log", "--oneline", "origin/main")
+	for _, want := range []string{"seed state", "concurrent write", "local change"} {
+		if !strings.Contains(log, want) {
+			t.Fatalf("expected origin/main history to contain %q after retry, got:\n%s", want, log)
+		}
+	}
+}
+
+// TestPushWithRebaseRetry_AbortsRebaseOnConflict proves the exported push helper
+// aborts the rebase and returns the wrapped error, leaving no mid-rebase state,
+// when the pull --rebase conflicts. WithDir targets the seed clone directly.
+func TestPushWithRebaseRetry_AbortsRebaseOnConflict(t *testing.T) {
+	// The concurrent writer changes the same file, so the rebase conflicts.
+	seed, _ := sharedRemoteClones(t, "state.txt", "base\n", "state.txt", "remote change\n")
+
+	if err := os.WriteFile(filepath.Join(seed, "state.txt"), []byte("local change\n"), 0o600); err != nil {
+		t.Fatalf("write local file: %v", err)
+	}
+	gitAt(t, seed, "add", "state.txt")
+	gitAt(t, seed, "commit", "-m", "local change")
+
+	if err := PushWithRebaseRetry(WithDir(seed), WithBackoff(time.Millisecond)); err == nil {
+		t.Fatal("PushWithRebaseRetry with a conflicting remote: expected error, got nil")
+	}
+
+	for _, name := range []string{"rebase-merge", "rebase-apply"} {
+		if _, statErr := os.Stat(filepath.Join(seed, ".git", name)); statErr == nil {
+			t.Fatalf("repository left mid-rebase: .git/%s still present", name)
+		}
+	}
+}
+
+// runGitOut runs a git command in dir and returns its trimmed stdout, failing the
+// test on error.
+func runGitOut(t *testing.T, dir string, args ...string) string {
+	t.Helper()
+	out, err := exec.Command("git", append([]string{"-C", dir}, args...)...).Output()
+	if err != nil {
+		t.Fatalf("git -C %s %s: %v", dir, strings.Join(args, " "), err)
+	}
+	return string(out)
+}
+
 // tagHead creates a lightweight tag pointing at the current HEAD.
 func tagHead(t *testing.T, name string) {
 	t.Helper()

internal/orchestrate/orchestrator.go

@@ -24,21 +24,11 @@ type Orchestrator struct {
 	cicdFile    *config.CICDFile
 	baseDir     string
 	// pushBackoff is the delay between state-write push retries. A zero value
-	// selects the default (defaultPushBackoff); tests override it to keep the
-	// retry loop fast.
+	// selects the shared git package default; tests override it to keep the retry
+	// loop fast. It is threaded into git.PushWithRebaseRetry via git.WithBackoff.
 	pushBackoff time.Duration
 }
 
-// State-write push retry policy. commitAndPush retries a rejected (for example
-// non-fast-forward) push behind a rebase so a concurrent state writer or a
-// "[skip ci]" commit that advances trunk between checkout and push does not fail
-// the run outright. This mirrors git.CommitAndPushWithRetry, the plain-git retry
-// path the promote and hotfix finalizers use for the manifest state write.
-const (
-	pushMaxAttempts    = 3
-	defaultPushBackoff = 2 * time.Second
-)
-
 // DefaultStateKey is used for state tracking when no environments are configured.
 const DefaultStateKey = "prerelease"
 
@@ -537,41 +527,16 @@ func (o *Orchestrator) commitAndPush(version string) error {
 // pushStateWithRetry pushes the committed state change, rebasing onto the
 // upstream and retrying when the push is rejected (for example a non-fast-forward
 // caused by a concurrent state writer or a "[skip ci]" commit landing on trunk
-// between checkout and push). It mirrors git.CommitAndPushWithRetry so the
-// orchestrator state write and the promote/hotfix finalize state write share the
-// same optimistic push behaviour.
+// between checkout and push). It delegates to git.PushWithRebaseRetry, the single
+// rebase-retry implementation the promote and hotfix finalizers also use, run
+// against the orchestrator's base directory so both state-write paths share one
+// rebase-abort-on-conflict behaviour.
 func (o *Orchestrator) pushStateWithRetry() error {
-	backoff := o.pushBackoff
-	if backoff == 0 {
-		backoff = defaultPushBackoff
-	}
-
-	var lastErr error
-	for attempt := 0; attempt < pushMaxAttempts; attempt++ {
-		lastErr = o.gitRun("push")
-		if lastErr == nil {
-			log.Info("Committed and pushed state changes")
-			return nil
-		}
-
-		if attempt == pushMaxAttempts-1 {
-			break
-		}
-
-		// Integrate the advanced upstream and replay the state commit on top,
-		// then retry the push.
-		if err := o.gitRun("pull", "--rebase"); err != nil {
-			// A failed rebase (typically a conflict) leaves the repository
-			// mid-rebase. Abort it so we neither leave a conflicted state
-			// behind nor loop into a guaranteed-failing push, and surface the
-			// real error instead of a generic push-failed summary.
-			_ = o.gitRun("rebase", "--abort") // best effort; nothing to abort is fine
-			return fmt.Errorf("git pull --rebase before push retry failed: %w", err)
-		}
-		time.Sleep(backoff)
+	if err := git.PushWithRebaseRetry(git.WithDir(o.baseDir), git.WithBackoff(o.pushBackoff)); err != nil {
+		return err
 	}
-
-	return fmt.Errorf("failed to push state changes after %d attempts: %w", pushMaxAttempts, lastErr)
+	log.Info("Committed and pushed state changes")
+	return nil
 }
 
 // gitOutput runs a git command and returns stdout.