fix: address roborev-ci review findings

- validateRefineContext: upstream resolution now happens only when
  --since is empty. --since provides the merge base explicitly, so
  an unfetched or configured-but-missing upstream must not block a
  valid --since invocation.
- tryBranchReview: treat UpstreamMissingError as "skip branch review"
  (return "", false) instead of silently falling back to the default
  branch. Hooks must never block commits, but falling back after a
  missing-but-configured upstream would enqueue a review against the
  wrong commit range in fork workflows.

Author: cpcloud

cmd/roborev/refine.go

@@ -237,30 +237,6 @@ func validateRefineContext(
 			)
 	}
 
-	// Prefer the current branch's upstream tracking ref (matches
-	// `git status` "ahead of upstream" semantics); fall back to the
-	// repository default branch when no upstream is configured.
-	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 && upstream != "" {
-		base = upstream
-	}
-	if base == "" {
-		base, 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
@@ -273,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 "", "", "", "",
@@ -299,6 +278,30 @@ func validateRefineContext(
 				)
 		}
 	} else {
+		// Prefer the current branch's upstream tracking ref (matches
+		// `git status` "ahead of upstream" semantics); fall back to the
+		// repository default branch when no upstream is configured.
+		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 && upstream != "" {
+			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 "+

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")

cmd/roborev/review.go

@@ -494,10 +494,16 @@ func tryBranchReview(root, baseBranchOverride string) (string, bool) {
 	base := baseBranchOverride
 	if base == "" {
 		// Prefer the branch's upstream tracking ref — matches `git status`
-		// semantics. Hooks must never block commits, so any error here —
-		// including an unresolvable @{upstream} — falls through silently to
-		// the default-branch lookup.
-		if upstream, err := git.GetUpstream(root, "HEAD"); err == nil && upstream != "" {
+		// semantics. Hooks must never block commits, but a configured-yet-
+		// unresolvable upstream means we cannot confidently pick a base;
+		// falling back to the default branch here would enqueue a review
+		// against the wrong commit range in fork workflows. Skip instead.
+		upstream, err := git.GetUpstream(root, "HEAD")
+		var missing *git.UpstreamMissingError
+		if errors.As(err, &missing) {
+			return "", false
+		}
+		if err == nil && upstream != "" {
 			base = upstream
 		}
 	}

cmd/roborev/review_test.go

@@ -611,6 +611,27 @@ func TestTryBranchReview(t *testing.T) {
 		assert.False(t, ok)
 	})
 
+	t.Run("skips when upstream is configured but unresolvable", func(t *testing.T) {
+		// Regression: when @{upstream} is configured but the remote-tracking
+		// ref has not been fetched, tryBranchReview must skip rather than
+		// silently fall back to origin/main or local main — that fallback
+		// would enqueue a review against the wrong commit range in fork
+		// workflows.
+		repo := newTestGitRepo(t)
+		repo.Run("symbolic-ref", "HEAD", "refs/heads/main")
+		repo.CommitFile("file.txt", "content", "initial")
+		repo.Run("checkout", "-b", "feature")
+		repo.CommitFile("feature.txt", "feature", "feature commit")
+		// Configure tracking against an upstream that never resolves locally.
+		repo.Run("config", "branch.feature.remote", "upstream")
+		repo.Run("config", "branch.feature.merge", "refs/heads/main")
+		writeRoborevConfig(t, repo, `post_commit_review = "branch"`)
+
+		ref, ok := tryBranchReview(repo.Dir, "")
+		assert.False(t, ok, "must skip when upstream is configured but missing")
+		assert.Empty(t, ref)
+	})
+
 	t.Run("blocks local main tracking non-origin upstream", func(t *testing.T) {
 		// Regression: LocalBranchName only stripped "origin/", so
 		// current="main" vs base="upstream/main" missed the guardrail