From 723edea122849579464c0ea82cfadd28a3fbf9fb Mon Sep 17 00:00:00 2001 From: ibolton336 Date: Fri, 7 Aug 2026 09:32:26 -0400 Subject: [PATCH 1/2] :bug: Skip branch push when a run produces no commits MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The harness pushed the target branch ref unconditionally on exit, so no-op, refused, and skill-less runs littered real repositories with one empty branch per run — multiplied by bulk workflow fan-out. Push now compares HEAD against the SHA captured after clone/checkout and skips, with an explicit log line, when the run produced nothing. Signed-off-by: ibolton336 Co-Authored-By: Claude Fable 5 --- changes/unreleased/skip-empty-push.yaml | 6 + harness/cmd/migration-harness/main.go | 13 +- harness/internal/git/git.go | 26 +++- harness/internal/git/git_test.go | 174 +++++++++++++++++++++++- 4 files changed, 214 insertions(+), 5 deletions(-) create mode 100644 changes/unreleased/skip-empty-push.yaml diff --git a/changes/unreleased/skip-empty-push.yaml b/changes/unreleased/skip-empty-push.yaml new file mode 100644 index 00000000..4eae7d93 --- /dev/null +++ b/changes/unreleased/skip-empty-push.yaml @@ -0,0 +1,6 @@ +kind: bugfix +description: > + Harness skips the branch push when a run produced no commits beyond its + checkout point, so no-op, refused, and skill-less runs no longer litter + remote repositories with empty branches. A workflow stage that adds no + new commits on the shared branch skips its push the same way. diff --git a/harness/cmd/migration-harness/main.go b/harness/cmd/migration-harness/main.go index d8c0196c..17a3b34c 100644 --- a/harness/cmd/migration-harness/main.go +++ b/harness/cmd/migration-harness/main.go @@ -124,6 +124,15 @@ func runStage(cmd *cobra.Command, args []string) error { } logging.Ok("cloned to %s, branch %s", cloneDir, creds.Branch) + // Base of this stage run: everything past this commit is work the run + // produced. Push compares HEAD against it so runs that produce no + // commits do not create empty remote branches. + baseSHA, err := git.HeadSHA(repo) + if err != nil { + // Fail open — an unknown base must never block a push of real work. + logging.Warn("resolve base commit: %v", err) + } + // 4. Discover skills early — controls which setup steps run skillPaths, err := discoverSkills() if err != nil { @@ -295,7 +304,7 @@ func runStage(cmd *cobra.Command, args []string) error { // 8. Start filesystem watcher BEFORE blocking prompt pushFn := func() error { return emitPush("git push (auto-commit watcher)", func() error { - return git.Push(ctx, creds, repo, creds.Branch) + return git.Push(ctx, creds, repo, creds.Branch, baseSHA) }) } w, err := watcher.New(cloneDir, pushFn) @@ -356,7 +365,7 @@ func runStage(cmd *cobra.Command, args []string) error { pushCtx, pushCancel := context.WithTimeout(context.Background(), 25*time.Second) defer pushCancel() if err := emitPush("git push (final)", func() error { - return git.Push(pushCtx, creds, repo, creds.Branch) + return git.Push(pushCtx, creds, repo, creds.Branch, baseSHA) }); err != nil { emitNotice("stage failed — final push error: %v", err) return fmt.Errorf("final push: %w", err) diff --git a/harness/internal/git/git.go b/harness/internal/git/git.go index 52e2c7e6..19a70dc2 100644 --- a/harness/internal/git/git.go +++ b/harness/internal/git/git.go @@ -12,6 +12,8 @@ import ( gogit "github.com/go-git/go-git/v5" gogitcfg "github.com/go-git/go-git/v5/config" "github.com/go-git/go-git/v5/plumbing" + + "github.com/konveyor/migration-harness/internal/logging" ) func isChildOf(path, root string) bool { @@ -187,7 +189,29 @@ func CheckoutBranch(repo *gogit.Repository, branch string) error { return nil } -func Push(ctx context.Context, cred *Credentials, repo *gogit.Repository, branch string) error { +// HeadSHA returns the hash of the current HEAD commit. +func HeadSHA(repo *gogit.Repository) (string, error) { + head, err := repo.Head() + if err != nil { + return "", fmt.Errorf("resolve HEAD: %w", err) + } + return head.Hash().String(), nil +} + +// Push updates refs/heads/ on origin. baseSHA is the commit the +// run started from (HEAD after clone/checkout): when HEAD still equals +// it the run produced no commits and the push is skipped, so no-op runs +// do not litter the remote with empty branches. An empty baseSHA +// disables the check — an unknown base must never block a push of real +// work. +func Push(ctx context.Context, cred *Credentials, repo *gogit.Repository, branch, baseSHA string) error { + if baseSHA != "" { + if head, err := repo.Head(); err == nil && head.Hash().String() == baseSHA { + logging.Info("no commits produced; skipping push of %s", branch) + return nil + } + } + refSpec := gogitcfg.RefSpec(fmt.Sprintf("refs/heads/%s:refs/heads/%s", branch, branch)) err := repo.PushContext(ctx, &gogit.PushOptions{ diff --git a/harness/internal/git/git_test.go b/harness/internal/git/git_test.go index 74388bac..5ae5e301 100644 --- a/harness/internal/git/git_test.go +++ b/harness/internal/git/git_test.go @@ -151,6 +151,11 @@ func TestFullLifecycle(t *testing.T) { t.Fatalf("CheckoutBranch: %v", err) } + baseSHA, err := HeadSHA(repo) + if err != nil { + t.Fatalf("HeadSHA: %v", err) + } + os.WriteFile(filepath.Join(cloneDir, "migrated.java"), []byte("class Foo {}\n"), 0644) wt, err := repo.Worktree() if err != nil { @@ -169,7 +174,7 @@ func TestFullLifecycle(t *testing.T) { t.Error("expected commit hash") } - if err := Push(ctx, cred, repo, cred.Branch); err != nil { + if err := Push(ctx, cred, repo, cred.Branch, baseSHA); err != nil { t.Fatalf("Push: %v", err) } @@ -207,6 +212,7 @@ func TestPushWithoutCredsToStrippedRemoteFails(t *testing.T) { StripCredentials(repo) CheckoutBranch(repo, cred.Branch) + baseSHA, _ := HeadSHA(repo) os.WriteFile(filepath.Join(cloneDir, "file.txt"), []byte("data\n"), 0644) wt, _ := repo.Worktree() @@ -216,7 +222,7 @@ func TestPushWithoutCredsToStrippedRemoteFails(t *testing.T) { }) // Push with nil credentials should fail - err = Push(ctx, &Credentials{RepoURL: remoteDir, Branch: cred.Branch}, repo, cred.Branch) + err = Push(ctx, &Credentials{RepoURL: remoteDir, Branch: cred.Branch}, repo, cred.Branch, baseSHA) // For local bare repos, push still works without auth — this test verifies // the function runs without panic. Real auth enforcement is server-side. _ = err @@ -294,3 +300,167 @@ func TestCommitFilesNoChanges(t *testing.T) { t.Error("HEAD changed despite no files to commit") } } + +func TestPushSkipsWhenNoNewCommits(t *testing.T) { + remoteDir, _ := setupBareRemote(t) + seedBareRepo(t, remoteDir) + + cred := &Credentials{ + Username: "test", + Token: "token", + RepoURL: remoteDir, + Branch: "migration-noop", + } + + ctx := context.Background() + cloneDir := filepath.Join(t.TempDir(), "work") + repo, err := Clone(ctx, cred, cloneDir) + if err != nil { + t.Fatalf("Clone: %v", err) + } + if err := CheckoutBranch(repo, cred.Branch); err != nil { + t.Fatalf("CheckoutBranch: %v", err) + } + baseSHA, err := HeadSHA(repo) + if err != nil { + t.Fatalf("HeadSHA: %v", err) + } + + // No commits beyond the checkout point: the push must be skipped so + // no-op runs do not create empty branches on the remote. + if err := Push(ctx, cred, repo, cred.Branch, baseSHA); err != nil { + t.Fatalf("Push: %v", err) + } + + remoteRepo, err := gogit.PlainOpen(remoteDir) + if err != nil { + t.Fatalf("open remote repo: %v", err) + } + if ref, err := remoteRepo.Reference(plumbing.NewBranchReferenceName(cred.Branch), false); err == nil { + t.Errorf("remote branch %s created at %s despite no commits", cred.Branch, ref.Hash()) + } +} + +func TestPushWithNewCommitsPushes(t *testing.T) { + remoteDir, _ := setupBareRemote(t) + seedBareRepo(t, remoteDir) + + cred := &Credentials{ + Username: "test", + Token: "token", + RepoURL: remoteDir, + Branch: "migration-work", + } + + ctx := context.Background() + cloneDir := filepath.Join(t.TempDir(), "work") + repo, err := Clone(ctx, cred, cloneDir) + if err != nil { + t.Fatalf("Clone: %v", err) + } + if err := CheckoutBranch(repo, cred.Branch); err != nil { + t.Fatalf("CheckoutBranch: %v", err) + } + baseSHA, err := HeadSHA(repo) + if err != nil { + t.Fatalf("HeadSHA: %v", err) + } + + os.WriteFile(filepath.Join(cloneDir, "migrated.java"), []byte("class Foo {}\n"), 0644) + wt, _ := repo.Worktree() + wt.Add("migrated.java") + hash, err := wt.Commit("migrate: Foo.java", &gogit.CommitOptions{ + Author: &object.Signature{Name: "test", Email: "test@test.com", When: time.Now()}, + }) + if err != nil { + t.Fatalf("commit: %v", err) + } + + if err := Push(ctx, cred, repo, cred.Branch, baseSHA); err != nil { + t.Fatalf("Push: %v", err) + } + + remoteRepo, err := gogit.PlainOpen(remoteDir) + if err != nil { + t.Fatalf("open remote repo: %v", err) + } + ref, err := remoteRepo.Reference(plumbing.NewBranchReferenceName(cred.Branch), false) + if err != nil { + t.Fatalf("remote branch not found: %v", err) + } + if ref.Hash() != hash { + t.Errorf("remote hash = %s, want %s", ref.Hash(), hash) + } +} + +func TestPushSkipsWhenStageAddsNoNewCommits(t *testing.T) { + remoteDir, _ := setupBareRemote(t) + seedBareRepo(t, remoteDir) + + cred := &Credentials{ + Username: "test", + Token: "token", + RepoURL: remoteDir, + Branch: "migration-stages", + } + ctx := context.Background() + + // Stage 1: commit on the branch and push — creates the remote ref. + stage1Dir := filepath.Join(t.TempDir(), "stage1") + repo1, err := Clone(ctx, cred, stage1Dir) + if err != nil { + t.Fatalf("stage 1 Clone: %v", err) + } + if err := CheckoutBranch(repo1, cred.Branch); err != nil { + t.Fatalf("stage 1 CheckoutBranch: %v", err) + } + base1, err := HeadSHA(repo1) + if err != nil { + t.Fatalf("stage 1 HeadSHA: %v", err) + } + os.WriteFile(filepath.Join(stage1Dir, "PLAN.md"), []byte("# Plan\n"), 0644) + wt, _ := repo1.Worktree() + wt.Add("PLAN.md") + hash, err := wt.Commit("plan stage", &gogit.CommitOptions{ + Author: &object.Signature{Name: "test", Email: "test@test.com", When: time.Now()}, + }) + if err != nil { + t.Fatalf("stage 1 commit: %v", err) + } + if err := Push(ctx, cred, repo1, cred.Branch, base1); err != nil { + t.Fatalf("stage 1 Push: %v", err) + } + + // Stage 2: fresh clone, checkout resolves the branch stage 1 pushed, + // so this stage's base is that tip. No new commits — skip the push. + stage2Dir := filepath.Join(t.TempDir(), "stage2") + repo2, err := Clone(ctx, cred, stage2Dir) + if err != nil { + t.Fatalf("stage 2 Clone: %v", err) + } + if err := CheckoutBranch(repo2, cred.Branch); err != nil { + t.Fatalf("stage 2 CheckoutBranch: %v", err) + } + base2, err := HeadSHA(repo2) + if err != nil { + t.Fatalf("stage 2 HeadSHA: %v", err) + } + if base2 != hash.String() { + t.Fatalf("stage 2 base = %s, want stage 1 tip %s", base2, hash) + } + if err := Push(ctx, cred, repo2, cred.Branch, base2); err != nil { + t.Fatalf("stage 2 Push: %v", err) + } + + remoteRepo, err := gogit.PlainOpen(remoteDir) + if err != nil { + t.Fatalf("open remote repo: %v", err) + } + ref, err := remoteRepo.Reference(plumbing.NewBranchReferenceName(cred.Branch), false) + if err != nil { + t.Fatalf("remote branch not found: %v", err) + } + if ref.Hash() != hash { + t.Errorf("remote hash = %s, want stage 1 tip %s", ref.Hash(), hash) + } +} From 06e1ab7edb1afc5b557a64cc06e54c8354afd12d Mon Sep 17 00:00:00 2001 From: ibolton336 Date: Wed, 12 Aug 2026 12:48:36 -0400 Subject: [PATCH 2/2] :bug: Report skipped pushes honestly; document the branch-creation guard Review follow-ups from #114: Push now returns whether it actually pushed, and the run notices use it so a skipped push reads "no changes to push" instead of claiming results landed on the branch. The failure/cancel notices only mention partial work when a push happened. A comment above the baseSHA guard records why it is not redundant with the NoErrAlreadyUpToDate swallow: on a missing remote branch PushContext sends a create command, so that error never fires and the guard is the only thing preventing empty branch creation. Co-Authored-By: Claude Fable 5 Signed-off-by: ibolton336 --- changes/unreleased/skip-empty-push.yaml | 4 +++- harness/cmd/migration-harness/main.go | 31 ++++++++++++++++-------- harness/internal/git/git.go | 16 +++++++++---- harness/internal/git/git_test.go | 32 ++++++++++++++++++++----- 4 files changed, 61 insertions(+), 22 deletions(-) diff --git a/changes/unreleased/skip-empty-push.yaml b/changes/unreleased/skip-empty-push.yaml index 4eae7d93..5e4b1813 100644 --- a/changes/unreleased/skip-empty-push.yaml +++ b/changes/unreleased/skip-empty-push.yaml @@ -3,4 +3,6 @@ description: > Harness skips the branch push when a run produced no commits beyond its checkout point, so no-op, refused, and skill-less runs no longer litter remote repositories with empty branches. A workflow stage that adds no - new commits on the shared branch skips its push the same way. + new commits on the shared branch skips its push the same way. Run + notices report a skipped push honestly (no changes to push) instead of + claiming results landed on the branch. diff --git a/harness/cmd/migration-harness/main.go b/harness/cmd/migration-harness/main.go index 17a3b34c..5297334f 100644 --- a/harness/cmd/migration-harness/main.go +++ b/harness/cmd/migration-harness/main.go @@ -264,7 +264,7 @@ func runStage(cmd *cobra.Command, args []string) error { }) } var pushSeq atomic.Int64 - emitPush := func(title string, fn func() error) error { + emitPush := func(title string, fn func() (bool, error)) (bool, error) { if teeSrv == nil { return fn() } @@ -273,7 +273,7 @@ func runStage(cmd *cobra.Command, args []string) error { "sessionUpdate": "tool_call", "toolCallId": id, "title": title, "kind": "execute", "status": "in_progress", }) - err := fn() + pushed, err := fn() status := "completed" if err != nil { status = "failed" @@ -281,7 +281,7 @@ func runStage(cmd *cobra.Command, args []string) error { teeSrv.EmitRunUpdate(map[string]any{ "sessionUpdate": "tool_call_update", "toolCallId": id, "status": status, }) - return err + return pushed, err } emitNotice := func(format string, args ...any) { if teeSrv == nil { @@ -303,9 +303,10 @@ func runStage(cmd *cobra.Command, args []string) error { // 8. Start filesystem watcher BEFORE blocking prompt pushFn := func() error { - return emitPush("git push (auto-commit watcher)", func() error { + _, err := emitPush("git push (auto-commit watcher)", func() (bool, error) { return git.Push(ctx, creds, repo, creds.Branch, baseSHA) }) + return err } w, err := watcher.New(cloneDir, pushFn) if err != nil { @@ -364,27 +365,37 @@ func runStage(cmd *cobra.Command, args []string) error { logging.Header("Final Push") pushCtx, pushCancel := context.WithTimeout(context.Background(), 25*time.Second) defer pushCancel() - if err := emitPush("git push (final)", func() error { + pushed, err := emitPush("git push (final)", func() (bool, error) { return git.Push(pushCtx, creds, repo, creds.Branch, baseSHA) - }); err != nil { + }) + if err != nil { emitNotice("stage failed — final push error: %v", err) return fmt.Errorf("final push: %w", err) } emitPlan("completed", "completed", "completed") - // 15. Exit + // 15. Exit. pushed is false when the run produced no commits, so the + // notices must not claim work landed on the branch. if stageFailed { switch { - case cancelled: + case cancelled && pushed: emitNotice("run cancelled by viewer — partial work pushed to branch %s", creds.Branch) - default: + case cancelled: + emitNotice("run cancelled by viewer — no commits to push") + case pushed: emitNotice("stage failed — partial work pushed to branch %s", creds.Branch) + default: + emitNotice("stage failed — no commits to push") } logging.Err("stage failed") return fmt.Errorf("stage failed") } stageSucceeded = true - emitNotice("stage succeeded — results pushed to branch %s", creds.Branch) + if pushed { + emitNotice("stage succeeded — results pushed to branch %s", creds.Branch) + } else { + emitNotice("stage succeeded — no changes to push") + } logging.Ok("stage succeeded") return nil } diff --git a/harness/internal/git/git.go b/harness/internal/git/git.go index 19a70dc2..76e96728 100644 --- a/harness/internal/git/git.go +++ b/harness/internal/git/git.go @@ -203,12 +203,18 @@ func HeadSHA(repo *gogit.Repository) (string, error) { // it the run produced no commits and the push is skipped, so no-op runs // do not litter the remote with empty branches. An empty baseSHA // disables the check — an unknown base must never block a push of real -// work. -func Push(ctx context.Context, cred *Credentials, repo *gogit.Repository, branch, baseSHA string) error { +// work. The returned bool is false only when the push was skipped, so +// callers can report the absence of results instead of claiming they +// landed on the branch. +func Push(ctx context.Context, cred *Credentials, repo *gogit.Repository, branch, baseSHA string) (bool, error) { + // Not redundant with the NoErrAlreadyUpToDate swallow below: when the + // remote branch does not exist yet, PushContext sends a create command + // (ZeroHash → local HEAD) instead of reporting already-up-to-date, so + // this guard is the only thing preventing empty branch creation. if baseSHA != "" { if head, err := repo.Head(); err == nil && head.Hash().String() == baseSHA { logging.Info("no commits produced; skipping push of %s", branch) - return nil + return false, nil } } @@ -220,8 +226,8 @@ func Push(ctx context.Context, cred *Credentials, repo *gogit.Repository, branch RefSpecs: []gogitcfg.RefSpec{refSpec}, }) if err != nil && !errors.Is(err, gogit.NoErrAlreadyUpToDate) { - return fmt.Errorf("push %s: %w", branch, err) + return false, fmt.Errorf("push %s: %w", branch, err) } - return nil + return true, nil } diff --git a/harness/internal/git/git_test.go b/harness/internal/git/git_test.go index 5ae5e301..3458bfcd 100644 --- a/harness/internal/git/git_test.go +++ b/harness/internal/git/git_test.go @@ -174,9 +174,13 @@ func TestFullLifecycle(t *testing.T) { t.Error("expected commit hash") } - if err := Push(ctx, cred, repo, cred.Branch, baseSHA); err != nil { + pushed, err := Push(ctx, cred, repo, cred.Branch, baseSHA) + if err != nil { t.Fatalf("Push: %v", err) } + if !pushed { + t.Error("Push reported skipped despite new commits") + } // Verify the branch exists on the remote remoteRepo, err := gogit.PlainOpen(remoteDir) @@ -222,7 +226,7 @@ func TestPushWithoutCredsToStrippedRemoteFails(t *testing.T) { }) // Push with nil credentials should fail - err = Push(ctx, &Credentials{RepoURL: remoteDir, Branch: cred.Branch}, repo, cred.Branch, baseSHA) + _, err = Push(ctx, &Credentials{RepoURL: remoteDir, Branch: cred.Branch}, repo, cred.Branch, baseSHA) // For local bare repos, push still works without auth — this test verifies // the function runs without panic. Real auth enforcement is server-side. _ = err @@ -328,9 +332,13 @@ func TestPushSkipsWhenNoNewCommits(t *testing.T) { // No commits beyond the checkout point: the push must be skipped so // no-op runs do not create empty branches on the remote. - if err := Push(ctx, cred, repo, cred.Branch, baseSHA); err != nil { + pushed, err := Push(ctx, cred, repo, cred.Branch, baseSHA) + if err != nil { t.Fatalf("Push: %v", err) } + if pushed { + t.Error("Push reported pushed despite no new commits") + } remoteRepo, err := gogit.PlainOpen(remoteDir) if err != nil { @@ -376,9 +384,13 @@ func TestPushWithNewCommitsPushes(t *testing.T) { t.Fatalf("commit: %v", err) } - if err := Push(ctx, cred, repo, cred.Branch, baseSHA); err != nil { + pushed, err := Push(ctx, cred, repo, cred.Branch, baseSHA) + if err != nil { t.Fatalf("Push: %v", err) } + if !pushed { + t.Error("Push reported skipped despite new commits") + } remoteRepo, err := gogit.PlainOpen(remoteDir) if err != nil { @@ -427,9 +439,13 @@ func TestPushSkipsWhenStageAddsNoNewCommits(t *testing.T) { if err != nil { t.Fatalf("stage 1 commit: %v", err) } - if err := Push(ctx, cred, repo1, cred.Branch, base1); err != nil { + pushed1, err := Push(ctx, cred, repo1, cred.Branch, base1) + if err != nil { t.Fatalf("stage 1 Push: %v", err) } + if !pushed1 { + t.Error("stage 1 Push reported skipped despite new commits") + } // Stage 2: fresh clone, checkout resolves the branch stage 1 pushed, // so this stage's base is that tip. No new commits — skip the push. @@ -448,9 +464,13 @@ func TestPushSkipsWhenStageAddsNoNewCommits(t *testing.T) { if base2 != hash.String() { t.Fatalf("stage 2 base = %s, want stage 1 tip %s", base2, hash) } - if err := Push(ctx, cred, repo2, cred.Branch, base2); err != nil { + pushed2, err := Push(ctx, cred, repo2, cred.Branch, base2) + if err != nil { t.Fatalf("stage 2 Push: %v", err) } + if pushed2 { + t.Error("stage 2 Push reported pushed despite no new commits") + } remoteRepo, err := gogit.PlainOpen(remoteDir) if err != nil {