fix(git): resolve stale local base to remote-tracking ref

cmd/roborev/review_test.go

@@ -474,6 +474,38 @@ func TestReviewBranchFlag(t *testing.T) {
 		req := <-reqCh
 		assert.Equal(t, mainSHA+"..HEAD", req.GitRef)
 	})
+
+	t.Run("branch review uses origin counterpart when local main is stale after rebase", func(t *testing.T) {
+		// Regression: a feature branch rebased onto origin/main with a stale
+		// local main pointing at an older commit must review only the new
+		// branch commits — not the trunk commits that already landed on
+		// origin/main between the local main pointer and the rebase target.
+		repo, mux := setupTestEnvironment(t)
+		reqCh := mockEnqueue(t, mux)
+
+		repo.Run("symbolic-ref", "HEAD", "refs/heads/main")
+		repo.CommitFile("base.txt", "base", "initial")
+		staleMainSHA := repo.Run("rev-parse", "HEAD")
+		// Two commits land on the trunk after local main was last updated.
+		repo.CommitFile("trunk1.txt", "t1", "trunk advance 1")
+		repo.CommitFile("trunk2.txt", "t2", "trunk advance 2")
+		freshOriginSHA := repo.Run("rev-parse", "HEAD")
+		// Rewind local main to simulate a stale checkout.
+		repo.Run("update-ref", "refs/heads/main", staleMainSHA)
+		repo.Run("remote", "add", "origin", "/dev/null")
+		repo.Run("update-ref", "refs/remotes/origin/main", freshOriginSHA)
+		// Feature branch was rebased onto origin/main.
+		repo.Run("checkout", "-b", "feature", freshOriginSHA)
+		repo.CommitFile("feature.txt", "f", "feature commit")
+		repo.Run("config", "branch.feature.base", "main")
+
+		_, _, err := executeReviewCmd("--repo", repo.Dir, "--branch")
+		require.NoError(t, err)
+
+		req := <-reqCh
+		assert.Equal(t, freshOriginSHA+"..HEAD", req.GitRef,
+			"range must start at origin/main, not stale local main")
+	})
 }
 
 func TestReviewFastFlag(t *testing.T) {
@@ -715,6 +747,31 @@ func TestTryBranchReview(t *testing.T) {
 		assert.False(t, ok, "local main tracking upstream/main must be treated as base branch")
 	})
 
+	t.Run("uses origin counterpart when local base is stale ancestor", func(t *testing.T) {
+		// Mirrors the rebase-onto-origin/main scenario for the post-commit
+		// hook path (tryBranchReview): branch.<feature>.base = main with a
+		// stale local main must resolve to origin/main, not the rewound
+		// pointer, so the hook enqueues only the real branch commits.
+		repo := newTestGitRepo(t)
+		repo.Run("symbolic-ref", "HEAD", "refs/heads/main")
+		repo.CommitFile("base.txt", "base", "initial")
+		staleMainSHA := repo.Run("rev-parse", "HEAD")
+		repo.CommitFile("trunk.txt", "t", "trunk advance")
+		freshOriginSHA := repo.Run("rev-parse", "HEAD")
+		repo.Run("update-ref", "refs/heads/main", staleMainSHA)
+		repo.Run("remote", "add", "origin", "/dev/null")
+		repo.Run("update-ref", "refs/remotes/origin/main", freshOriginSHA)
+		repo.Run("checkout", "-b", "feature", freshOriginSHA)
+		repo.CommitFile("feature.txt", "f", "feature commit")
+		repo.Run("config", "branch.feature.base", "main")
+		writeRoborevConfig(t, repo, `post_commit_review = "branch"`)
+
+		ref, ok := tryBranchReview(repo.Dir, "")
+		require.True(t, ok, "expected branch review with stale local main")
+		assert.Equal(t, freshOriginSHA+"..HEAD", ref,
+			"range must start at origin/main, not stale local main")
+	})
+
 	t.Run("prefers branch upstream over default branch", func(t *testing.T) {
 		// Reproduces the "single commit past upstream" bug: when the default
 		// branch (e.g., origin/main) lags behind the tip the branch actually

internal/git/git.go

@@ -1353,14 +1353,97 @@ func readUpstreamConfig(repoPath, ref string) (upstreamConfig, bool) {
 	}, true
 }
 
-// GetBranchBase returns roborev's branch.<name>.base config value for ref.
-// Passing an empty ref is equivalent to HEAD.
+// GetBranchBase returns the base ref derived from ref's branch.<name>.base
+// config. Bare names (e.g. "main") are resolved to the remote-tracking ref
+// (e.g. "origin/main") that the local branch tracks when local <name> exists
+// and is an ancestor of the remote-tracking ref — i.e., local <name> is just
+// a stale snapshot of the trunk. Without this, merge-base(local-main, HEAD)
+// walks back to the stale pointer and the resulting range pulls in commits
+// already merged into the remote trunk after a rebase-onto-origin/main.
+// Qualified refs (containing '/') are returned as-is so users can pin to
+// e.g. "upstream/main". Passing an empty ref is equivalent to HEAD.
 func GetBranchBase(repoPath, ref string) string {
 	branch := branchNameForConfig(repoPath, ref)
 	if branch == "" {
 		return ""
 	}
-	return readGitConfig(repoPath, "branch."+branch+".base")
+	raw := readGitConfig(repoPath, "branch."+branch+".base")
+	return resolveConfiguredBase(repoPath, raw)
+}
+
+// resolveConfiguredBase translates a configured base name to the remote-
+// tracking ref it should map to, or returns configValue unchanged when no
+// translation applies. Values that already name a remote-tracking ref
+// unambiguously are passed through; local branch names (including slash-
+// containing names like "release/1.2") are subject to stale-ancestor
+// translation; divergent local branches are also passed through.
+func resolveConfiguredBase(repoPath, configValue string) string {
+	if configValue == "" {
+		return configValue
+	}
+	if isQualifiedRemoteRef(repoPath, configValue) {
+		return configValue
+	}
+	remoteTracking := preferredRemoteTracking(repoPath, configValue)
+	if remoteTracking == "" {
+		return configValue
+	}
+	if !refExists(repoPath, "refs/heads/"+configValue) {
+		return remoteTracking
+	}
+	isAncestor, err := IsAncestor(repoPath, configValue, remoteTracking)
+	if err != nil || !isAncestor {
+		return configValue
+	}
+	return remoteTracking
+}
+
+// isQualifiedRemoteRef reports whether value names a remote-tracking ref
+// that is not shadowed by a same-named local branch. Defers to the local
+// branch when both exist so stale-ancestor translation still applies, and
+// so the resolved base matches git's normal ref lookup precedence
+// (refs/heads beats refs/remotes).
+func isQualifiedRemoteRef(repoPath, value string) bool {
+	if !strings.Contains(value, "/") {
+		return false
+	}
+	if !refExists(repoPath, "refs/remotes/"+value) {
+		return false
+	}
+	if refExists(repoPath, "refs/heads/"+value) {
+		return false
+	}
+	return true
+}
+
+// preferredRemoteTracking returns the remote-tracking ref short name for a
+// branch name, preferring the branch's configured @{upstream} so fork
+// workflows (local main → upstream/main) translate correctly, and falling
+// back to origin/<name> only when no upstream is configured. Returns ""
+// when no remote-tracking counterpart resolves locally; in particular:
+//   - An explicitly configured upstream that does not resolve is NOT
+//     silently substituted with origin/<name> — that would override the
+//     user's choice of remote in fork workflows.
+//   - For slash-containing names, the origin/<name> fallback only applies
+//     when a local branch by that name exists. Otherwise a remote-
+//     qualified value like "upstream/main" whose configured remote-
+//     tracking ref isn't fetched would collapse to "origin/upstream/main",
+//     silently switching to a different remote.
+func preferredRemoteTracking(repoPath, name string) string {
+	if cfg, ok := readUpstreamConfig(repoPath, name); ok {
+		if strings.HasPrefix(cfg.qualified, "refs/remotes/") &&
+			refExists(repoPath, cfg.qualified) {
+			return cfg.short
+		}
+		return ""
+	}
+	if !refExists(repoPath, "refs/remotes/origin/"+name) {
+		return ""
+	}
+	if strings.Contains(name, "/") && !refExists(repoPath, "refs/heads/"+name) {
+		return ""
+	}
+	return "origin/" + name
 }
 
 func branchNameForConfig(repoPath, ref string) string {

internal/git/git_test.go

@@ -2087,3 +2087,216 @@ func TestLooksLikeSHA(t *testing.T) {
 		})
 	}
 }
+
+func TestGetBranchBase(t *testing.T) {
+	t.Run("returns empty when not configured", func(t *testing.T) {
+		repo := NewTestRepo(t)
+		repo.Run("symbolic-ref", "HEAD", "refs/heads/main")
+		repo.CommitFile("base.txt", "base", "initial")
+		repo.Run("checkout", "-b", "feature")
+
+		assert.Empty(t, GetBranchBase(repo.Dir, "HEAD"))
+	})
+
+	t.Run("returns local bare name when no remote-tracking exists", func(t *testing.T) {
+		repo := NewTestRepo(t)
+		repo.Run("symbolic-ref", "HEAD", "refs/heads/main")
+		repo.CommitFile("base.txt", "base", "initial")
+		repo.Run("checkout", "-b", "feature")
+		repo.Run("config", "branch.feature.base", "main")
+
+		assert.Equal(t, "main", GetBranchBase(repo.Dir, "HEAD"))
+	})
+
+	t.Run("resolves bare name to origin counterpart when local is stale ancestor", func(t *testing.T) {
+		// Reproduces the post-rebase bug: branch.<feature>.base = main with
+		// a stale local main pulls extra origin/main commits into the merge-
+		// base..HEAD range. The fix translates the bare name to origin/main
+		// when local main is just an ancestor of origin/main.
+		repo := NewTestRepo(t)
+		repo.Run("symbolic-ref", "HEAD", "refs/heads/main")
+		repo.CommitFile("base.txt", "base", "initial")
+		staleMainSHA := repo.HeadSHA()
+		repo.CommitFile("trunk.txt", "trunk", "trunk advance")
+		freshOriginSHA := repo.HeadSHA()
+		// Rewind local main so it lags one commit behind origin/main.
+		repo.Run("update-ref", "refs/heads/main", staleMainSHA)
+		repo.Run("remote", "add", "origin", "/dev/null")
+		repo.Run("update-ref", "refs/remotes/origin/main", freshOriginSHA)
+		repo.Run("checkout", "-b", "feature", freshOriginSHA)
+		repo.CommitFile("feature.txt", "f", "feature commit")
+		repo.Run("config", "branch.feature.base", "main")
+
+		assert.Equal(t, "origin/main", GetBranchBase(repo.Dir, "HEAD"))
+	})
+
+	t.Run("keeps local name when local has diverged from origin", func(t *testing.T) {
+		// Divergence (local main has commits not on origin/main) means the
+		// user's local branch is not just stale — respect their config
+		// rather than silently picking a different base.
+		repo := NewTestRepo(t)
+		repo.Run("symbolic-ref", "HEAD", "refs/heads/main")
+		repo.CommitFile("base.txt", "base", "initial")
+		baseSHA := repo.HeadSHA()
+		repo.CommitFile("origin.txt", "origin", "origin-only commit")
+		originMainSHA := repo.HeadSHA()
+		// Local main diverges with its own commit not present on origin/main.
+		repo.Run("update-ref", "refs/heads/main", baseSHA)
+		repo.Run("checkout", "main")
+		repo.CommitFile("local.txt", "local", "local-only commit")
+		repo.Run("remote", "add", "origin", "/dev/null")
+		repo.Run("update-ref", "refs/remotes/origin/main", originMainSHA)
+		repo.Run("checkout", "-b", "feature")
+		repo.Run("config", "branch.feature.base", "main")
+
+		assert.Equal(t, "main", GetBranchBase(repo.Dir, "HEAD"))
+	})
+
+	t.Run("returns origin counterpart when local branch is missing", func(t *testing.T) {
+		repo := NewTestRepo(t)
+		repo.Run("symbolic-ref", "HEAD", "refs/heads/scratch")
+		repo.CommitFile("base.txt", "base", "initial")
+		originSHA := repo.HeadSHA()
+		repo.Run("remote", "add", "origin", "/dev/null")
+		repo.Run("update-ref", "refs/remotes/origin/main", originSHA)
+		// Note: no refs/heads/main. Branch from origin/main directly.
+		repo.Run("checkout", "-b", "feature", originSHA)
+		repo.CommitFile("feature.txt", "f", "feature commit")
+		repo.Run("config", "branch.feature.base", "main")
+
+		assert.Equal(t, "origin/main", GetBranchBase(repo.Dir, "HEAD"))
+	})
+
+	t.Run("returns qualified ref unchanged", func(t *testing.T) {
+		repo := NewTestRepo(t)
+		repo.Run("symbolic-ref", "HEAD", "refs/heads/main")
+		repo.CommitFile("base.txt", "base", "initial")
+		repo.Run("checkout", "-b", "feature")
+		repo.Run("config", "branch.feature.base", "upstream/main")
+
+		assert.Equal(t, "upstream/main", GetBranchBase(repo.Dir, "HEAD"))
+	})
+
+	t.Run("resolves slash-containing local branch when stale", func(t *testing.T) {
+		// Branch names may contain slashes (release/1.2, team/main). Such
+		// names must still get stale-ancestor translation — treating any
+		// '/' as a remote prefix would skip the fix for these cases.
+		repo := NewTestRepo(t)
+		repo.Run("symbolic-ref", "HEAD", "refs/heads/release/1.2")
+		repo.CommitFile("base.txt", "base", "initial")
+		staleSHA := repo.HeadSHA()
+		repo.CommitFile("trunk.txt", "t", "trunk advance")
+		freshSHA := repo.HeadSHA()
+		repo.Run("update-ref", "refs/heads/release/1.2", staleSHA)
+		repo.Run("remote", "add", "origin", "/dev/null")
+		repo.Run("update-ref", "refs/remotes/origin/release/1.2", freshSHA)
+		repo.Run("checkout", "-b", "feature", freshSHA)
+		repo.CommitFile("feature.txt", "f", "feature commit")
+		repo.Run("config", "branch.feature.base", "release/1.2")
+
+		assert.Equal(t, "origin/release/1.2", GetBranchBase(repo.Dir, "HEAD"))
+	})
+
+	t.Run("passes qualified remote-tracking ref through unchanged", func(t *testing.T) {
+		// An unambiguous remote-tracking ref (refs/remotes/<value> exists,
+		// no shadowing local branch) is not subject to translation.
+		repo := NewTestRepo(t)
+		repo.Run("symbolic-ref", "HEAD", "refs/heads/main")
+		repo.CommitFile("base.txt", "base", "initial")
+		mainSHA := repo.HeadSHA()
+		repo.Run("remote", "add", "upstream", "/dev/null")
+		repo.Run("update-ref", "refs/remotes/upstream/main", mainSHA)
+		repo.Run("checkout", "-b", "feature")
+		repo.Run("config", "branch.feature.base", "upstream/main")
+
+		assert.Equal(t, "upstream/main", GetBranchBase(repo.Dir, "HEAD"))
+	})
+
+	t.Run("does not fall back to origin when configured upstream is missing", func(t *testing.T) {
+		// Fork workflow: local main is configured to track upstream/main
+		// but upstream/main has not been fetched. origin/main (the user's
+		// fork) must NOT be silently substituted — that would compute the
+		// merge-base against a different remote than the user intended.
+		repo := NewTestRepo(t)
+		repo.Run("symbolic-ref", "HEAD", "refs/heads/main")
+		repo.CommitFile("base.txt", "base", "initial")
+		mainSHA := repo.HeadSHA()
+		repo.Run("remote", "add", "origin", "/dev/null")
+		repo.Run("update-ref", "refs/remotes/origin/main", mainSHA)
+		// Configure local main to track upstream/main, but never set up
+		// the corresponding remote-tracking ref.
+		repo.Run("config", "branch.main.remote", "upstream")
+		repo.Run("config", "branch.main.merge", "refs/heads/main")
+		repo.Run("checkout", "-b", "feature")
+		repo.Run("config", "branch.feature.base", "main")
+
+		assert.Equal(t, "main", GetBranchBase(repo.Dir, "HEAD"),
+			"missing upstream must not be substituted with origin counterpart")
+	})
+
+	t.Run("does not collapse remote-qualified value to origin counterpart", func(t *testing.T) {
+		// Regression: branch.feature.base = "upstream/main" with the
+		// upstream remote-tracking ref absent must not be rewritten to
+		// "origin/upstream/main" just because a stray refs/remotes/origin/
+		// upstream/main happens to exist. The user explicitly remote-
+		// qualified the value; either it resolves or git surfaces an error.
+		repo := NewTestRepo(t)
+		repo.Run("symbolic-ref", "HEAD", "refs/heads/main")
+		repo.CommitFile("base.txt", "base", "initial")
+		mainSHA := repo.HeadSHA()
+		repo.Run("remote", "add", "origin", "/dev/null")
+		// origin holds a stray ref shaped like a remote-qualified path —
+		// e.g., a past push of a local branch literally named
+		// "upstream/main". refs/remotes/upstream/main remains absent.
+		repo.Run("update-ref", "refs/remotes/origin/upstream/main", mainSHA)
+		repo.Run("checkout", "-b", "feature")
+		repo.Run("config", "branch.feature.base", "upstream/main")
+
+		assert.Equal(t, "upstream/main", GetBranchBase(repo.Dir, "HEAD"),
+			"explicit remote-qualified value must not collapse to origin/<value>")
+	})
+
+	t.Run("does not fall back to origin when configured upstream is local-only", func(t *testing.T) {
+		// Local-branch tracking (branch.<name>.remote = ".") explicitly
+		// targets another local branch. origin/<name> is unrelated and
+		// must not be silently substituted.
+		repo := NewTestRepo(t)
+		repo.Run("symbolic-ref", "HEAD", "refs/heads/develop")
+		repo.CommitFile("base.txt", "base", "initial")
+		developSHA := repo.HeadSHA()
+		repo.Run("checkout", "-b", "main")
+		repo.Run("config", "branch.main.remote", ".")
+		repo.Run("config", "branch.main.merge", "refs/heads/develop")
+		repo.Run("remote", "add", "origin", "/dev/null")
+		repo.Run("update-ref", "refs/remotes/origin/main", developSHA)
+		repo.Run("checkout", "-b", "feature")
+		repo.Run("config", "branch.feature.base", "main")
+
+		assert.Equal(t, "main", GetBranchBase(repo.Dir, "HEAD"),
+			"local-branch tracking must not be substituted with origin counterpart")
+	})
+
+	t.Run("prefers configured upstream over origin counterpart", func(t *testing.T) {
+		// Fork workflow: local main tracks upstream/main, and origin/main
+		// also exists (e.g., points at the user's fork). The base should
+		// resolve to upstream/main because that's what local main tracks.
+		repo := NewTestRepo(t)
+		repo.Run("symbolic-ref", "HEAD", "refs/heads/main")
+		repo.CommitFile("base.txt", "base", "initial")
+		mainSHA := repo.HeadSHA()
+		repo.CommitFile("trunk.txt", "trunk", "upstream advance")
+		upstreamSHA := repo.HeadSHA()
+		repo.Run("update-ref", "refs/heads/main", mainSHA)
+		repo.Run("remote", "add", "origin", "/dev/null")
+		repo.Run("remote", "add", "upstream", "/dev/null")
+		repo.Run("update-ref", "refs/remotes/origin/main", mainSHA)
+		repo.Run("update-ref", "refs/remotes/upstream/main", upstreamSHA)
+		repo.Run("config", "branch.main.remote", "upstream")
+		repo.Run("config", "branch.main.merge", "refs/heads/main")
+		repo.Run("checkout", "-b", "feature", upstreamSHA)
+		repo.CommitFile("feature.txt", "f", "feature commit")
+		repo.Run("config", "branch.feature.base", "main")
+
+		assert.Equal(t, "upstream/main", GetBranchBase(repo.Dir, "HEAD"))
+	})
+}