fix: use branch upstream (not default branch) for --branch reviews (#660)

## Summary

- `review --branch`, `tryBranchReview`, `analyze --branch`, and `refine` resolved the merge-base against the repository's default branch (typically `origin/main`). In fork workflows where the current branch's actual upstream (e.g. `upstream/main`) is ahead of `origin/main`, this pulled in commits that were already merged upstream. Each command now prefers `@{upstream}` of the current branch and falls back to `GetDefaultBranch()` only when no upstream is configured.
- Added `git.GetUpstream(repoPath, ref)` (returns the upstream tracking branch via `git rev-parse --abbrev-ref <ref>@{upstream}`, empty on no upstream).
- Replaced the `currentBranch == LocalBranchName(base)` guardrails with `git.IsOnBaseBranch(repoPath, currentBranch, base)`, which generalizes the `origin/` shortcut by verifying `refs/remotes/<base>` exists before stripping the remote prefix — so non-origin remotes are handled and local branches with slashes (e.g. `feature/foo`) are not misclassified.
- New tests: `TestGetUpstream`, `TestIsOnBaseBranch`, and `TestTryBranchReview` cases for the non-origin upstream scenario and for the feature/foo misclassification guard.

Co-authored-by: Phillip Cloud <cpcloud@users.noreply.github.com>

Author: cpcloud

cmd/roborev/analyze.go

@@ -4,6 +4,7 @@ import (
 	"bytes"
 	"context"
 	"encoding/json"
+	"errors"
 	"fmt"
 	"io"
 	"net/http"
@@ -967,6 +968,25 @@ func getBranchFiles(cmd *cobra.Command, repoRoot string, opts analyzeOptions) (m
 
 	// Determine base branch
 	base := opts.baseBranch
+	if base == "" {
+		// Prefer the branch's upstream tracking ref only when it resolves to
+		// a trunk-named branch (e.g., local main tracking upstream/main in a
+		// fork). A branch tracking its own remote counterpart (feature
+		// tracking origin/feature) is not trunk — using it would skip
+		// already-pushed feature commits, contradicting "--branch analyzes
+		// all commits since trunk".
+		upstream, uerr := git.GetUpstream(repoRoot, targetRef)
+		var missing *git.UpstreamMissingError
+		if errors.As(uerr, &missing) {
+			return nil, fmt.Errorf("%w (or pass --base <ref>)", missing)
+		}
+		if uerr != nil {
+			return nil, fmt.Errorf("resolve upstream for %s: %w (pass --base <ref> to skip)", targetRef, uerr)
+		}
+		if upstream != "" && git.UpstreamIsTrunk(repoRoot, targetRef) {
+			base = upstream
+		}
+	}
 	if base == "" {
 		var err error
 		base, err = git.GetDefaultBranch(repoRoot)
@@ -978,8 +998,8 @@ func getBranchFiles(cmd *cobra.Command, repoRoot string, opts analyzeOptions) (m
 	// Validate not on base branch (only when analyzing current branch)
 	if targetRef == "HEAD" {
 		currentBranch := git.GetCurrentBranch(repoRoot)
-		if currentBranch == git.LocalBranchName(base) {
-			return nil, fmt.Errorf("already on %s - switch to a feature branch first", git.LocalBranchName(base))
+		if git.IsOnBaseBranch(repoRoot, currentBranch, base) {
+			return nil, fmt.Errorf("already on %s - switch to a feature branch first", currentBranch)
 		}
 	}
 

cmd/roborev/analyze_test.go

@@ -570,6 +570,51 @@ func TestGetBranchFiles(t *testing.T) {
 		require.Error(t, err, "expected error when on base branch")
 		assert.Contains(t, err.Error(), "already on main", "unexpected error")
 	})
+
+	t.Run("prefers non-origin upstream as base", func(t *testing.T) {
+		// Fork workflow: origin (stale fork) and upstream (real repo).
+		// Feature branch tracks upstream/main; merge-base must be taken
+		// against upstream/main, not origin/main, so commits already
+		// merged upstream aren't re-analyzed.
+		remote := newBareTestGitRepo(t)
+		stale := newBareTestGitRepo(t)
+		repo := newTestGitRepo(t)
+		repo.Run("symbolic-ref", "HEAD", "refs/heads/main")
+		repo.CommitFile("a.go", "package main", "u1")
+		repo.CommitFile("b.go", "package main", "u2")
+		repo.Run("remote", "add", "upstream", remote.Dir)
+		repo.Run("push", "-u", "upstream", "main")
+
+		// origin lags upstream by 1 commit.
+		repo.Run("remote", "add", "origin", stale.Dir)
+		repo.Run("push", "origin", "HEAD~1:refs/heads/main")
+		repo.Run("fetch", "origin")
+		repo.Run("remote", "set-head", "origin", "main")
+
+		repo.Run("checkout", "-b", "feature", "--track", "upstream/main")
+		repo.CommitFile("only-new.go", "package main\nfunc New() {}", "feature work")
+
+		files, err := getBranchFiles(cmd, repo.Dir, analyzeOptions{branch: "HEAD"})
+		require.NoError(t, err)
+		// b.go was already merged to upstream/main; must not be analyzed.
+		assert.NotContains(t, files, "b.go",
+			"already-merged upstream commit must not be analyzed")
+		assert.Contains(t, files, "only-new.go",
+			"new feature file should be analyzed")
+	})
+
+	t.Run("blocks local main tracking non-origin upstream", func(t *testing.T) {
+		remote := newBareTestGitRepo(t)
+		repo := newTestGitRepo(t)
+		repo.Run("symbolic-ref", "HEAD", "refs/heads/main")
+		repo.CommitFile("a.go", "package main", "initial")
+		repo.Run("remote", "add", "upstream", remote.Dir)
+		repo.Run("push", "-u", "upstream", "main")
+
+		_, err := getBranchFiles(cmd, repo.Dir, analyzeOptions{branch: "HEAD"})
+		require.Error(t, err, "expected error when on base branch")
+		assert.Contains(t, err.Error(), "already on main", "unexpected error")
+	})
 }
 
 func TestAnalyzeBranchSpaceSeparated(t *testing.T) {

cmd/roborev/helpers_test.go

@@ -43,6 +43,21 @@ func newTestGitRepo(t *testing.T) *TestGitRepo {
 	return r
 }
 
+// newBareTestGitRepo creates a bare git repository for use as a remote.
+func newBareTestGitRepo(t *testing.T) *TestGitRepo {
+	t.Helper()
+	if _, err := exec.LookPath("git"); err != nil {
+		t.Skip("git not available")
+	}
+	dir := t.TempDir()
+	resolved, err := filepath.EvalSymlinks(dir)
+	require.NoError(t, err, "Failed to resolve symlinks: %v")
+
+	r := &TestGitRepo{Dir: resolved, t: t}
+	r.Run("init", "--bare")
+	return r
+}
+
 // chdir changes to dir and registers a t.Cleanup to restore the original directory.
 func chdir(t *testing.T, dir string) {
 	t.Helper()

cmd/roborev/refine.go

@@ -207,12 +207,14 @@ func (t *stepTimer) stopLive() {
 }
 
 // validateRefineContext validates git and branch preconditions for refine.
-// Returns repoPath, currentBranch, defaultBranch, mergeBase, or an error.
+// Returns repoPath, currentBranch, base, mergeBase, or an error.
+// base is the ref that HEAD was diverged from — the branch's upstream
+// tracking ref when configured, otherwise the repository's default branch.
 // If branchFlag is non-empty, validates that the user is on that branch.
 // This validation happens before any daemon interaction.
 func validateRefineContext(
 	cwd, since, branchFlag string,
-) (repoPath, currentBranch, defaultBranch, mergeBase string, err error) {
+) (repoPath, currentBranch, base, mergeBase string, err error) {
 	repoPath, err = git.GetRepoRoot(cwd)
 	if err != nil {
 		return "", "", "", "",
@@ -235,14 +237,6 @@ func validateRefineContext(
 			)
 	}
 
-	defaultBranch, err = git.GetDefaultBranch(repoPath)
-	if err != nil {
-		return "", "", "", "",
-			fmt.Errorf(
-				"cannot determine default branch: %w", err,
-			)
-	}
-
 	currentBranch = git.GetCurrentBranch(repoPath)
 
 	// --branch: validate the user is on the expected branch
@@ -255,6 +249,9 @@ func validateRefineContext(
 	}
 
 	if since != "" {
+		// --since provides an explicit merge base, so upstream/default-branch
+		// resolution is unnecessary. Skip it so a misconfigured or unfetched
+		// upstream doesn't block an otherwise-valid --since invocation.
 		mergeBase, err = git.ResolveSHA(repoPath, since)
 		if err != nil {
 			return "", "", "", "",
@@ -281,27 +278,58 @@ func validateRefineContext(
 				)
 		}
 	} else {
-		if currentBranch == git.LocalBranchName(defaultBranch) {
+		// Prefer the current branch's upstream tracking ref only when it
+		// resolves to a trunk-named branch (e.g., local main tracking
+		// upstream/main in a fork). A branch tracking its own remote
+		// counterpart is not trunk — use GetDefaultBranch instead.
+		upstream, uerr := git.GetUpstream(repoPath, "HEAD")
+		var missing *git.UpstreamMissingError
+		if errors.As(uerr, &missing) {
+			return "", "", "", "",
+				fmt.Errorf(
+					"%w (run 'git fetch' or pass --since)", missing,
+				)
+		}
+		if uerr != nil {
+			return "", "", "", "",
+				fmt.Errorf(
+					"resolve upstream for HEAD: %w (pass --since to skip)", uerr,
+				)
+		}
+		if upstream != "" && git.UpstreamIsTrunk(repoPath, "HEAD") {
+			base = upstream
+		}
+		if base == "" {
+			base, err = git.GetDefaultBranch(repoPath)
+			if err != nil {
+				return "", "", "", "",
+					fmt.Errorf(
+						"cannot determine default branch: %w", err,
+					)
+			}
+		}
+
+		if git.IsOnBaseBranch(repoPath, currentBranch, base) {
 			return "", "", "", "", fmt.Errorf(
 				"refusing to refine on %s branch "+
 					"without --since flag",
-				git.LocalBranchName(defaultBranch),
+				currentBranch,
 			)
 		}
 
 		mergeBase, err = git.GetMergeBase(
-			repoPath, defaultBranch, "HEAD",
+			repoPath, base, "HEAD",
 		)
 		if err != nil {
 			return "", "", "", "",
 				fmt.Errorf(
 					"cannot find merge-base with %s: %w",
-					defaultBranch, err,
+					base, err,
 				)
 		}
 	}
 
-	return repoPath, currentBranch, defaultBranch, mergeBase, nil
+	return repoPath, currentBranch, base, mergeBase, nil
 }
 
 // RunContext encapsulates the runtime context for the refine command,
@@ -314,7 +342,7 @@ type RunContext struct {
 
 func runRefine(ctx RunContext, opts refineOptions) error {
 	// 1. Validate git and branch context (before touching daemon)
-	repoPath, currentBranch, defaultBranch, mergeBase, err := validateRefineContext(
+	repoPath, currentBranch, base, mergeBase, err := validateRefineContext(
 		ctx.WorkingDir, opts.since, opts.branch,
 	)
 	if err != nil {
@@ -368,7 +396,7 @@ func runRefine(ctx RunContext, opts refineOptions) error {
 	if opts.since != "" {
 		fmt.Printf("Refining commits since %s on branch %q\n", git.ShortSHA(mergeBase), currentBranch)
 	} else {
-		fmt.Printf("Refining branch %q (diverged from %s at %s)\n", currentBranch, defaultBranch, git.ShortSHA(mergeBase))
+		fmt.Printf("Refining branch %q (diverged from %s at %s)\n", currentBranch, base, git.ShortSHA(mergeBase))
 	}
 
 	allowUnsafe := resolveAllowUnsafeAgents(opts.allowUnsafeAgents, opts.unsafeFlagChanged, cfg)

cmd/roborev/refine_test.go

@@ -11,6 +11,7 @@ import (
 	"github.com/roborev-dev/roborev/internal/agent"
 	"github.com/roborev-dev/roborev/internal/config"
 	"github.com/roborev-dev/roborev/internal/daemon"
+	"github.com/roborev-dev/roborev/internal/git"
 	"github.com/roborev-dev/roborev/internal/storage"
 	"github.com/roborev-dev/roborev/internal/testutil"
 	"github.com/stretchr/testify/assert"
@@ -563,6 +564,54 @@ func TestValidateRefineContext_AllowsMainBranchWithSince(t *testing.T) {
 	assert.Equal(t, mergeBase, baseSHA)
 }
 
+func TestValidateRefineContext_SinceIgnoresUpstreamMissing(t *testing.T) {
+	if _, err := exec.LookPath("git"); err != nil {
+		t.Skip("git not available")
+	}
+
+	// --since provides the merge base explicitly, so an unfetched or
+	// missing @{upstream} must not block a valid --since invocation.
+	repo := testutil.InitTestRepo(t)
+	baseSHA := repo.RevParse("HEAD")
+	repo.CommitFile("second.txt", "second", "second commit")
+
+	// Configure tracking against an upstream that never resolves locally.
+	repo.Run("config", "branch.main.remote", "upstream")
+	repo.Run("config", "branch.main.merge", "refs/heads/main")
+
+	chdirForTest(t, repo.Root)
+
+	repoPath, currentBranch, _, mergeBase, err := validateRefineContext("", baseSHA, "")
+	require.NoError(t, err, "--since should bypass upstream resolution: %v", err)
+
+	assert.NotEmpty(t, repoPath)
+	assert.Equal(t, "main", currentBranch)
+	assert.Equal(t, baseSHA, mergeBase)
+}
+
+func TestValidateRefineContext_UpstreamMissingErrorsWithoutSince(t *testing.T) {
+	if _, err := exec.LookPath("git"); err != nil {
+		t.Skip("git not available")
+	}
+
+	// Without --since, a configured-but-unfetched upstream must surface
+	// as UpstreamMissingError rather than silently falling back to the
+	// repository default branch (which could yield the wrong range).
+	repo := testutil.InitTestRepo(t)
+	repo.RunGit("checkout", "-b", "feature")
+	repo.CommitFile("feature.txt", "feature", "feature commit")
+	repo.Run("config", "branch.feature.remote", "upstream")
+	repo.Run("config", "branch.feature.merge", "refs/heads/main")
+
+	chdirForTest(t, repo.Root)
+
+	_, _, _, _, err := validateRefineContext("", "", "")
+	require.Error(t, err, "expected missing-upstream error without --since")
+
+	var missing *git.UpstreamMissingError
+	assert.ErrorAs(t, err, &missing, "expected UpstreamMissingError, got: %v", err)
+}
+
 func TestValidateRefineContext_SinceWorksOnFeatureBranch(t *testing.T) {
 	if _, err := exec.LookPath("git"); err != nil {
 		t.Skip("git not available")
@@ -619,6 +668,69 @@ func TestValidateRefineContext_SinceNotAncestorOfHEAD(t *testing.T) {
 	assert.Contains(t, err.Error(), "not an ancestor of HEAD")
 }
 
+func TestValidateRefineContext_PrefersNonOriginUpstream(t *testing.T) {
+	if _, err := exec.LookPath("git"); err != nil {
+		t.Skip("git not available")
+	}
+
+	// Fork workflow: origin (stale fork) lags behind upstream (real repo).
+	// Feature branch tracks upstream/main. validateRefineContext must
+	// pick upstream/main as the base so commits already merged upstream
+	// are not refined.
+	repo := testutil.InitTestRepo(t)
+	repo.CommitFile("upstream_c2.go", "package main", "upstream c2")
+	upstreamSHA := repo.RevParse("HEAD")
+
+	bareRemote := t.TempDir()
+	bareStale := t.TempDir()
+	repo.Run("init", "--bare", bareRemote)
+	repo.Run("init", "--bare", bareStale)
+	repo.Run("remote", "add", "upstream", bareRemote)
+	repo.Run("push", "-u", "upstream", "main")
+
+	repo.Run("remote", "add", "origin", bareStale)
+	repo.Run("push", "origin", "HEAD~1:refs/heads/main")
+	repo.Run("fetch", "origin")
+	repo.Run("remote", "set-head", "origin", "main")
+
+	repo.RunGit("checkout", "-b", "feature", "--track", "upstream/main")
+	repo.CommitFile("feature.go", "package main", "feature commit")
+
+	chdirForTest(t, repo.Root)
+
+	repoPath, currentBranch, base, mergeBase, err := validateRefineContext("", "", "")
+	require.NoError(t, err, "validation should pass on feature tracking upstream/main")
+
+	assert.NotEmpty(t, repoPath)
+	assert.Equal(t, "feature", currentBranch)
+	assert.Equal(t, "upstream/main", base,
+		"base must resolve to the branch's upstream, not origin/main")
+	assert.Equal(t, upstreamSHA, mergeBase,
+		"merge-base must be upstream/main's tip so already-merged commits aren't refined")
+}
+
+func TestValidateRefineContext_RefusesLocalMainTrackingNonOriginUpstream(t *testing.T) {
+	if _, err := exec.LookPath("git"); err != nil {
+		t.Skip("git not available")
+	}
+
+	// Local main tracking upstream/main must still hit the "refusing to
+	// refine on main" guardrail even though base is "upstream/main"
+	// (not "origin/main"). Regression for IsOnBaseBranch's non-origin
+	// remote handling.
+	repo := testutil.InitTestRepo(t)
+	bareRemote := t.TempDir()
+	repo.Run("init", "--bare", bareRemote)
+	repo.Run("remote", "add", "upstream", bareRemote)
+	repo.Run("push", "-u", "upstream", "main")
+
+	chdirForTest(t, repo.Root)
+
+	_, _, _, _, err := validateRefineContext("", "", "")
+	require.Error(t, err, "expected refuse-on-base error for main tracking upstream/main")
+	assert.Contains(t, err.Error(), "refusing to refine on main")
+}
+
 func TestValidateRefineContext_FeatureBranchWithoutSinceStillWorks(t *testing.T) {
 	if _, err := exec.LookPath("git"); err != nil {
 		t.Skip("git not available")