From bccd4d5f402863fbf018ad8f4158cfee167cd66d Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Carlos=20Flor=C3=AAncio?= Date: Fri, 3 Apr 2026 15:40:40 +0100 Subject: [PATCH] fix: always set upstream tracking on git push and fix task worktree startPoint (#536) --- .../internal/agentctl/server/process/git.go | 17 +++-- .../agentctl/server/process/git_test.go | 52 +++++++++++++ apps/backend/internal/worktree/manager.go | 6 +- .../manager_taskdir_checkout_branch_test.go | 76 +++++++++++++++++++ 4 files changed, 143 insertions(+), 8 deletions(-) create mode 100644 apps/backend/internal/agentctl/server/process/git_test.go create mode 100644 apps/backend/internal/worktree/manager_taskdir_checkout_branch_test.go diff --git a/apps/backend/internal/agentctl/server/process/git.go b/apps/backend/internal/agentctl/server/process/git.go index 6f55af446c..ab56f108f0 100644 --- a/apps/backend/internal/agentctl/server/process/git.go +++ b/apps/backend/internal/agentctl/server/process/git.go @@ -186,9 +186,9 @@ func (g *GitOperator) getCurrentBranch(ctx context.Context) (string, error) { return strings.TrimSpace(output), nil } -// getUpstreamRef returns the upstream tracking ref for the given branch, or "" if none is set. -func (g *GitOperator) getUpstreamRef(ctx context.Context, branch string) string { - output, err := g.runGitCommand(ctx, "rev-parse", "--abbrev-ref", "--symbolic-full-name", branch+"@{upstream}") +// getUpstreamRef returns the current branch's upstream tracking ref, or "" if none is set. +func (g *GitOperator) getUpstreamRef(ctx context.Context) string { + output, err := g.runGitCommand(ctx, "rev-parse", "--abbrev-ref", "--symbolic-full-name", "@{upstream}") if err != nil { return "" } @@ -259,7 +259,7 @@ func (g *GitOperator) Pull(ctx context.Context, rebase bool) (*GitOperationResul // Use upstream branch if set, otherwise fall back to default (main/master). // This handles local branches that haven't been pushed to the remote yet. pullBranch := branch - if upstream := g.getUpstreamRef(ctx, branch); upstream == "" { + if upstream := g.getUpstreamRef(ctx); upstream == "" { if defaultBranch := g.getDefaultRemoteBranch(ctx); defaultBranch != "" { pullBranch = defaultBranch } @@ -312,8 +312,8 @@ func (g *GitOperator) Push(ctx context.Context, force bool, setUpstream bool) (* } args := []string{"push"} - - if setUpstream { + shouldSetUpstream := setUpstream || g.getUpstreamRef(ctx) == "" + if shouldSetUpstream { args = append(args, "--set-upstream") } @@ -333,7 +333,10 @@ func (g *GitOperator) Push(ctx context.Context, force bool, setUpstream bool) (* } result.Success = true - g.logger.Info("push completed", zap.String("branch", branch), zap.Bool("force", force), zap.Bool("set_upstream", setUpstream)) + g.logger.Info("push completed", + zap.String("branch", branch), + zap.Bool("force", force), + zap.Bool("set_upstream", shouldSetUpstream)) return result, nil } diff --git a/apps/backend/internal/agentctl/server/process/git_test.go b/apps/backend/internal/agentctl/server/process/git_test.go new file mode 100644 index 0000000000..d74eace592 --- /dev/null +++ b/apps/backend/internal/agentctl/server/process/git_test.go @@ -0,0 +1,52 @@ +package process + +import ( + "context" + "path/filepath" + "strings" + "testing" +) + +func TestGitOperatorPush_PreservesExistingUpstream(t *testing.T) { + repoDir, cleanup := setupTestRepo(t) + defer cleanup() + + log := newTestLogger(t) + + runGit(t, repoDir, "checkout", "-b", "feature/pr-branch") + writeFile(t, repoDir, "feature.txt", "feature branch\n") + runGit(t, repoDir, "add", ".") + runGit(t, repoDir, "commit", "-m", "feature commit") + runGit(t, repoDir, "push", "-u", "origin", "feature/pr-branch") + + runGit(t, repoDir, "checkout", "main") + worktreeBase := t.TempDir() + directDir := filepath.Join(worktreeBase, "wt-direct") + suffixedDir := filepath.Join(worktreeBase, "wt-sfx") + runGit(t, repoDir, "worktree", "add", directDir, "feature/pr-branch") + runGit(t, repoDir, "worktree", "add", "-b", "feature/pr-branch-sfx", suffixedDir, "origin/feature/pr-branch") + + runGit(t, suffixedDir, "branch", "--set-upstream-to=origin/feature/pr-branch", "feature/pr-branch-sfx") + writeFile(t, suffixedDir, "feature.txt", "feature branch\nlocal change\n") + runGit(t, suffixedDir, "add", ".") + runGit(t, suffixedDir, "commit", "-m", "local change") + + before := strings.TrimSpace(runGit(t, suffixedDir, "rev-parse", "--abbrev-ref", "@{upstream}")) + if before != "origin/feature/pr-branch" { + t.Fatalf("upstream before push = %q, want %q", before, "origin/feature/pr-branch") + } + + gitOp := NewGitOperator(suffixedDir, log, nil) + result, err := gitOp.Push(context.Background(), false, false) + if err != nil { + t.Fatalf("Push returned error: %v", err) + } + if !result.Success { + t.Fatalf("Push failed: %+v", result) + } + + after := strings.TrimSpace(runGit(t, suffixedDir, "rev-parse", "--abbrev-ref", "@{upstream}")) + if after != "origin/feature/pr-branch" { + t.Fatalf("upstream after push = %q, want %q", after, "origin/feature/pr-branch") + } +} diff --git a/apps/backend/internal/worktree/manager.go b/apps/backend/internal/worktree/manager.go index e74241829f..4dc77a7007 100644 --- a/apps/backend/internal/worktree/manager.go +++ b/apps/backend/internal/worktree/manager.go @@ -342,7 +342,11 @@ func (m *Manager) createInTaskDir(ctx context.Context, req CreateRequest, baseRe if err != nil { return nil, err } - startPoint = req.CheckoutBranch + if fetchResult.StartPoint != "" { + startPoint = fetchResult.StartPoint + } else { + startPoint = req.CheckoutBranch + } } worktreeID, branchName, err := m.addWorktreeForBranch(ctx, req, worktreePath, branchName, startPoint, baseRef) diff --git a/apps/backend/internal/worktree/manager_taskdir_checkout_branch_test.go b/apps/backend/internal/worktree/manager_taskdir_checkout_branch_test.go new file mode 100644 index 0000000000..605f24e90c --- /dev/null +++ b/apps/backend/internal/worktree/manager_taskdir_checkout_branch_test.go @@ -0,0 +1,76 @@ +package worktree + +import ( + "context" + "strings" + "testing" +) + +func TestCreateInTaskDir_CheckoutBranchUsesRemoteStartPointAndUpstream(t *testing.T) { + cfg := newTestConfig(t) + cfg.TasksBasePath = cfg.BasePath + log := newTestLogger() + store := newMockStore() + + mgr, err := NewManager(cfg, store, log) + if err != nil { + t.Fatalf("NewManager failed: %v", err) + } + + repoPath := initGitRepoWithRemote(t) + + wt1, err := mgr.Create(context.Background(), CreateRequest{ + TaskID: "task-1", + SessionID: "session-1", + TaskTitle: "PR Review 1", + RepositoryID: "repo-1", + RepositoryPath: repoPath, + BaseBranch: "main", + CheckoutBranch: "feature/pr-branch", + TaskDirName: "task-1", + RepoName: "repo-1", + }) + if err != nil { + t.Fatalf("Create() first task-dir worktree: %v", err) + } + if wt1.Branch != "feature/pr-branch" { + t.Fatalf("first worktree branch = %q, want %q", wt1.Branch, "feature/pr-branch") + } + + wt2, err := mgr.Create(context.Background(), CreateRequest{ + TaskID: "task-2", + SessionID: "session-2", + TaskTitle: "PR Review 2", + RepositoryID: "repo-1", + RepositoryPath: repoPath, + BaseBranch: "main", + CheckoutBranch: "feature/pr-branch", + TaskDirName: "task-2", + RepoName: "repo-1", + }) + if err != nil { + t.Fatalf("Create() second task-dir worktree: %v", err) + } + if !strings.HasPrefix(wt2.Branch, "feature/pr-branch-") { + t.Fatalf("second worktree branch = %q, want feature/pr-branch-*", wt2.Branch) + } + if wt2.FetchWarning != "" { + t.Fatalf("second worktree fetch warning = %q, want empty", wt2.FetchWarning) + } + + sha1 := strings.TrimSpace(runGit(t, wt1.Path, "rev-parse", "HEAD")) + sha2 := strings.TrimSpace(runGit(t, wt2.Path, "rev-parse", "HEAD")) + if sha1 != sha2 { + t.Fatalf("worktree SHAs differ: wt1=%q wt2=%q", sha1, sha2) + } + + const wantUpstream = "origin/feature/pr-branch" + upstream1 := strings.TrimSpace(runGit(t, wt1.Path, "rev-parse", "--abbrev-ref", "@{upstream}")) + if upstream1 != wantUpstream { + t.Fatalf("first worktree upstream = %q, want %q", upstream1, wantUpstream) + } + upstream2 := strings.TrimSpace(runGit(t, wt2.Path, "rev-parse", "--abbrev-ref", "@{upstream}")) + if upstream2 != wantUpstream { + t.Fatalf("second worktree upstream = %q, want %q", upstream2, wantUpstream) + } +}