feat: fetch a force-pushed approval commit instead of dismissing blind

When a branch is rebased or force-pushed, the commit an approval points at
stops being reachable from any local ref. ChangesSince then fails on
git diff base...<approvalSHA>, and the approval lands in badApprovals with no
ownership check at all. That dismissal is not "your files changed", it is
"I could not tell, so I reset you".

GitHub still serves the object. With fetch_orphaned_approval on, a ref which
cannot be resolved locally is fetched from origin once and the diff retried,
so the approval is judged on its diff rather than lost to a rewritten branch.

Fail-safe throughout: if the fetch or the retry fails, the original diff error
stays the cause and the approval is dismissed exactly as before.

The fetch is bounded at a minute, since it is the only git call here that
waits on a remote, and the ref is passed after a -- so that a ref beginning
with a dash cannot be read as an option. git fetch accepts --upload-pack,
which names a command to run.

Author: asyncawaitpromise

README.md

@@ -366,6 +366,8 @@ fetch_orphaned_approval = false
 
 What counts as a change not worth re-reviewing is a judgement about a particular codebase, not something to inherit from a default. Two flags deserve extra thought before you name them. A change to a string literal or a rename can alter behavior without changing the shape of the code the approver reviewed, so retaining an approval across one is a stronger claim than the other categories. And `fetch_orphaned_approval` is the only flag in the section which reaches outside the checkout, so it adds network calls to a run.
 
+`fetch_orphaned_approval` is the one flag which is not about the content of a change. A rebase or a force-push can leave the commit an approval points at unreachable from every local ref, and a diff which cannot be computed dismisses that approval without ever asking who owns what changed. With this flag on, a ref which cannot be resolved locally is fetched from `origin` once and the diff retried, so the approval is judged on its diff rather than lost to a rewritten branch. It is the only flag in the section which reaches outside the checkout: every other one reads the diff already in hand, while this one costs a fetch - bounded at one minute - on each distinct approval commit which cannot be resolved. A branch which has been force-pushed several times can therefore carry several of those, and a remote which does not answer turns each into a minute of waiting. That cost is the reason the flag stays off unless it is asked for, rather than following the umbrella.
+
 #### Require Both Branch Reviewers (Ownership Handoffs)
 
 The `require_both_branch_reviewers` feature enables self-service ownership transfers by requiring approval from codeowners defined in **BOTH** the base branch and the PR branch. This creates an AND relationship between ownership rules from both branches.

internal/app/app.go

@@ -131,7 +131,11 @@ func (a *App) Run() (*OutputData, error) {
 
 	// Get the diff of the PR
 	a.printDebug("Getting diff for %s...%s\n", diffContext.Base, diffContext.Head)
-	gitDiff, err := git.NewDiff(diffContext)
+	var diffOpts []git.DiffOption
+	if conf.ApprovalRetention.FetchOrphanedApprovalEnabled() {
+		diffOpts = append(diffOpts, git.WithFetchOrphanedRefs())
+	}
+	gitDiff, err := git.NewDiff(diffContext, diffOpts...)
 	if err != nil {
 		return &OutputData{}, fmt.Errorf("NewGitDiff Error: %v", err)
 	}

internal/app/approval_retention_fetch_test.go

@@ -0,0 +1,110 @@
+package app
+
+import (
+	"os/exec"
+	"strings"
+	"testing"
+)
+
+const orphanBaseSource = `package service
+
+func Gamma() int {
+	return 3
+}
+`
+
+const orphanApprovedSource = `package service
+
+func Gamma() int {
+	return 30
+}
+`
+
+// The approved commit exists only on the remote, as a force-push leaves it, and the
+// head edits a file the reviewer does not own: the approval survives iff the diff does.
+func buildOrphanedApprovalRepo(t *testing.T, configBody string) (repoDir, baseSHA, headSHA, approvalSHA string) {
+	t.Helper()
+	repoDir = t.TempDir()
+	initRepo(t, repoDir)
+
+	writeRepoFile(t, repoDir, ".codeowners", "service.go @owner\n")
+	writeRepoFile(t, repoDir, "codeowners.toml", configBody)
+	writeRepoFile(t, repoDir, "service.go", orphanBaseSource)
+	writeRepoFile(t, repoDir, "notes.md", "first note\n")
+	baseSHA = commitAll(t, repoDir, "base")
+
+	// Grown on the remote, where the local repository never sees it.
+	originDir := t.TempDir()
+	runGit(t, repoDir, "clone", "-q", repoDir, originDir)
+	initRepo(t, originDir)
+	runGit(t, originDir, "config", "uploadpack.allowAnySHA1InWant", "true")
+	writeRepoFile(t, originDir, "service.go", orphanApprovedSource)
+	approvalSHA = commitAll(t, originDir, "approved change")
+
+	writeRepoFile(t, repoDir, "service.go", orphanApprovedSource)
+	writeRepoFile(t, repoDir, "notes.md", "first note\nsecond note\n")
+	headSHA = commitAll(t, repoDir, "approved change plus an unowned edit")
+	runGit(t, repoDir, "remote", "add", "origin", originDir)
+
+	return repoDir, baseSHA, headSHA, approvalSHA
+}
+
+// The config file alone decides whether an approval whose commit exists only on the
+// remote is recovered or dismissed.
+func TestRunFetchesOrphanedApproval(t *testing.T) {
+	const fetchOff = `disable_review_status_comments = true
+suppress_unowned_warning = true
+`
+	const fetchOn = `disable_review_status_comments = true
+suppress_unowned_warning = true
+
+[approval_retention]
+enabled = true
+fetch_orphaned_approval = true
+`
+
+	tt := []struct {
+		name            string
+		config          string
+		expectDismissed bool
+	}{
+		{name: "fetch disabled, approval dismissed", config: fetchOff, expectDismissed: true},
+		{name: "fetch enabled, approval recovered", config: fetchOn, expectDismissed: false},
+	}
+
+	for _, tc := range tt {
+		t.Run(tc.name, func(t *testing.T) {
+			repoDir, baseSHA, headSHA, approvalSHA := buildOrphanedApprovalRepo(t, tc.config)
+
+			// Guard the premise: if the commit resolved locally the fetch would have
+			// nothing to recover and both cases would pass for the wrong reason.
+			cmd := exec.Command("git", "cat-file", "-e", approvalSHA)
+			cmd.Dir = repoDir
+			if err := cmd.Run(); err == nil {
+				t.Fatalf("approval commit %s should not be present locally", approvalSHA)
+			}
+
+			output, dismissed, warnings := runApp(t, repoDir, baseSHA, headSHA, approvalSHA)
+
+			if tc.expectDismissed {
+				if len(dismissed) != 1 {
+					t.Errorf("expected the approval to be dismissed, got %d dismissals", len(dismissed))
+				}
+				if !strings.Contains(warnings, "Error getting changes since") {
+					t.Errorf("expected a warning about the unresolvable ref, got %q", warnings)
+				}
+				if output.Success {
+					t.Error("expected the run to fail without the approval")
+				}
+				return
+			}
+
+			if len(dismissed) != 0 {
+				t.Errorf("expected the approval to survive, got %d dismissals: %s", len(dismissed), warnings)
+			}
+			if !output.Success {
+				t.Errorf("expected the run to succeed, got %q (warnings: %s)", output.Message, warnings)
+			}
+		})
+	}
+}

internal/git/diff.go

@@ -3,21 +3,32 @@ package git
 import (
 	"bufio"
 	"bytes"
+	"context"
 	"crypto/sha256"
+	"errors"
 	"fmt"
 	"os/exec"
 	"slices"
 	"strings"
+	"time"
 
 	"github.com/multimediallc/codeowners-plus/pkg/codeowners"
 	"github.com/sourcegraph/go-diff/diff"
 )
 
+// fetch is the only git call here that waits on a remote, so it alone needs a bound.
+const fetchTimeout = 60 * time.Second
+
 // gitCommandExecutor defines the interface for executing git commands
 type gitCommandExecutor interface {
 	execute(command string, args ...string) ([]byte, error)
 }
 
+// timeoutExecutor is implemented only by executors that can bound a command.
+type timeoutExecutor interface {
+	executeWithTimeout(timeout time.Duration, command string, args ...string) ([]byte, error)
+}
+
 // realGitExecutor implements GitCommandExecutor using os/exec
 type realGitExecutor struct {
 	dir string
@@ -33,25 +44,50 @@ func (e *realGitExecutor) execute(command string, args ...string) ([]byte, error
 	return cmd.CombinedOutput()
 }
 
+func (e *realGitExecutor) executeWithTimeout(timeout time.Duration, command string, args ...string) ([]byte, error) {
+	ctx, cancel := context.WithTimeout(context.Background(), timeout)
+	defer cancel()
+	cmd := exec.CommandContext(ctx, command, args...)
+	cmd.Dir = e.dir
+	output, err := cmd.CombinedOutput()
+	if errors.Is(ctx.Err(), context.DeadlineExceeded) {
+		// The kill signal surfaces as a plain exit error, so name the real cause
+		return output, fmt.Errorf("%s timed out after %s", command, timeout)
+	}
+	return output, err
+}
+
 type Diff interface {
 	AllChanges() []codeowners.DiffFile
 	ChangesSince(ref string) ([]codeowners.DiffFile, error)
 	Context() DiffContext
 }
 
 type GitDiff struct {
-	context  DiffContext
-	diff     []*diff.FileDiff
-	files    []codeowners.DiffFile
-	executor gitCommandExecutor
+	context           DiffContext
+	diff              []*diff.FileDiff
+	files             []codeowners.DiffFile
+	executor          gitCommandExecutor
+	fetchOrphanedRefs bool
+}
+
+// DiffOption configures optional GitDiff behavior.
+type DiffOption func(*GitDiff)
+
+// WithFetchOrphanedRefs makes ChangesSince fetch a ref git cannot resolve locally
+// and retry once: a force-pushed approval commit is dismissed unreviewed otherwise.
+func WithFetchOrphanedRefs() DiffOption {
+	return func(gd *GitDiff) {
+		gd.fetchOrphanedRefs = true
+	}
 }
 
-func NewDiff(context DiffContext) (Diff, error) {
+func NewDiff(context DiffContext, opts ...DiffOption) (Diff, error) {
 	executor := newRealGitExecutor(context.Dir)
-	return NewDiffWithExecutor(context, executor)
+	return NewDiffWithExecutor(context, executor, opts...)
 }
 
-func NewDiffWithExecutor(context DiffContext, executor gitCommandExecutor) (Diff, error) {
+func NewDiffWithExecutor(context DiffContext, executor gitCommandExecutor, opts ...DiffOption) (Diff, error) {
 	gitDiff, err := getGitDiff(context, executor)
 	if err != nil {
 		return nil, err
@@ -61,12 +97,16 @@ func NewDiffWithExecutor(context DiffContext, executor gitCommandExecutor) (Diff
 		return nil, err
 	}
 
-	return &GitDiff{
+	gd := &GitDiff{
 		context:  context,
 		diff:     gitDiff,
 		files:    diffFiles,
 		executor: executor,
-	}, nil
+	}
+	for _, opt := range opts {
+		opt(gd)
+	}
+	return gd, nil
 }
 
 func (gd *GitDiff) AllChanges() []codeowners.DiffFile {
@@ -81,6 +121,17 @@ func (gd *GitDiff) ChangesSince(ref string) ([]codeowners.DiffFile, error) {
 		IgnoreDirs: gd.context.IgnoreDirs,
 	}
 	olderDiff, err := getGitDiff(olderDiffContext, gd.executor)
+	if err != nil && gd.fetchOrphanedRefs {
+		// A force-push leaves the ref on no local branch, but the remote still
+		// serves the object.
+		if fetchErr := gd.fetchRef(ref); fetchErr != nil {
+			err = fmt.Errorf("%w (fetching orphaned ref failed: %v)", err, fetchErr)
+		} else if retryDiff, retryErr := getGitDiff(olderDiffContext, gd.executor); retryErr != nil {
+			err = fmt.Errorf("%w (retry after fetching orphaned ref failed: %v)", err, retryErr)
+		} else {
+			olderDiff, err = retryDiff, nil
+		}
+	}
 	if err != nil {
 		return nil, fmt.Errorf("failed to get older diff: %w", err)
 	}
@@ -95,6 +146,18 @@ func (gd *GitDiff) ChangesSince(ref string) ([]codeowners.DiffFile, error) {
 	return diffFiles, nil
 }
 
+// The `--` stops a ref beginning with a dash being read as an option: `git fetch`
+// accepts --upload-pack, which names a command to run.
+func (gd *GitDiff) fetchRef(ref string) error {
+	args := []string{"fetch", "--no-tags", "origin", "--", ref}
+	if executor, ok := gd.executor.(timeoutExecutor); ok {
+		_, err := executor.executeWithTimeout(fetchTimeout, "git", args...)
+		return err
+	}
+	_, err := gd.executor.execute("git", args...)
+	return err
+}
+
 func (gd *GitDiff) Context() DiffContext {
 	return gd.context
 }