diff --git a/changes/unreleased/skip-empty-push.yaml b/changes/unreleased/skip-empty-push.yaml new file mode 100644 index 00000000..5e4b1813 --- /dev/null +++ b/changes/unreleased/skip-empty-push.yaml @@ -0,0 +1,8 @@ +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. 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 d8c0196c..5297334f 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 { @@ -255,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() } @@ -264,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" @@ -272,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 { @@ -294,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 { - return git.Push(ctx, creds, repo, creds.Branch) + _, 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 { @@ -355,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 { - return git.Push(pushCtx, creds, repo, creds.Branch) - }); err != nil { + pushed, err := emitPush("git push (final)", func() (bool, error) { + return git.Push(pushCtx, creds, repo, creds.Branch, baseSHA) + }) + 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 52e2c7e6..76e96728 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,35 @@ 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. 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 false, nil + } + } + refSpec := gogitcfg.RefSpec(fmt.Sprintf("refs/heads/%s:refs/heads/%s", branch, branch)) err := repo.PushContext(ctx, &gogit.PushOptions{ @@ -196,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 74388bac..3458bfcd 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,9 +174,13 @@ func TestFullLifecycle(t *testing.T) { t.Error("expected commit hash") } - if err := Push(ctx, cred, repo, cred.Branch); 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) @@ -207,6 +216,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 +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) + _, 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 +304,183 @@ 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. + 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 { + 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) + } + + 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 { + 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) + } + 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. + 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) + } + 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 { + 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) + } +}