feat: recover an approval whose commit no longer resolves locally

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 orphaned 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: if the fetch or the retry fails, the original diff error stays the
cause and the approval is dismissed exactly as before.

Hardening:
- The fetch is bounded at 60s. It is the only git call here that waits on a
  remote; every other one is local and returns promptly.
- The ref is passed after a `--` so a ref beginning with a dash cannot be read as
  an option. `git fetch` accepts --upload-pack, which names a command to run.
- The original diff error is preserved as the wrapped cause, with any fetch or
  retry failure appended.

Opt-in only, and default off so that enabling it is always a deliberate choice to
add network calls to a run.

Author: asyncawaitpromise

README.md

@@ -254,6 +254,13 @@ self_approval_via_teams = false
 # Optional reviewers are still invited with a CC comment.
 disable_review_status_comments = false
 
+# `fetch_orphaned_approval` (default false) recovers an approval whose commit no
+# longer resolves locally, which is what a rebase or a force-push leaves behind.
+# Without it that approval is dismissed with no ownership check at all, because the
+# diff it would be judged on cannot be computed. Off by default: it is the only
+# setting here that adds a network call, bounded at 60s per unresolvable commit.
+fetch_orphaned_approval = false
+
 # `enforcement` allows you to specify how the Codeowners Plus check should be enforced
 [enforcement]
 # see "Enforcement Options" below for more details

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.FetchOrphanedApproval {
+		diffOpts = append(diffOpts, git.WithFetchOrphanedRefs())
+	}
+	gitDiff, err := git.NewDiff(diffContext, diffOpts...)
 	if err != nil {
 		return &OutputData{}, fmt.Errorf("NewGitDiff Error: %v", err)
 	}

internal/app/orphaned_approval_test.go

@@ -0,0 +1,223 @@
+package app
+
+import (
+	"bytes"
+	"os"
+	"os/exec"
+	"path/filepath"
+	"strings"
+	"testing"
+
+	"github.com/google/go-github/v89/github"
+	"github.com/multimediallc/codeowners-plus/internal/git"
+	gh "github.com/multimediallc/codeowners-plus/internal/github"
+	"github.com/multimediallc/codeowners-plus/pkg/codeowners"
+)
+
+// The shared mock approves everything without reading the diff, which is the
+// decision under test, so the real staleness check is spliced back in.
+type realCheckApprovalsClient struct {
+	*mockGitHubClient
+	real      gh.Client
+	dismissed []*gh.CurrentApproval
+}
+
+func (c *realCheckApprovalsClient) CheckApprovals(
+	fileReviewerMap map[string][]string,
+	approvals []*gh.CurrentApproval,
+	originalDiff git.Diff,
+) ([]codeowners.Slug, []*gh.CurrentApproval) {
+	return c.real.CheckApprovals(fileReviewerMap, approvals, originalDiff)
+}
+
+func (c *realCheckApprovalsClient) DismissStaleReviews(approvals []*gh.CurrentApproval) error {
+	c.dismissed = append(c.dismissed, approvals...)
+	return c.mockGitHubClient.DismissStaleReviews(approvals)
+}
+
+func runGit(t *testing.T, dir string, args ...string) string {
+	t.Helper()
+	cmd := exec.Command("git", args...)
+	cmd.Dir = dir
+	cmd.Env = append(os.Environ(), "GIT_CONFIG_GLOBAL=/dev/null", "GIT_CONFIG_SYSTEM=/dev/null")
+	out, err := cmd.CombinedOutput()
+	if err != nil {
+		t.Fatalf("git %s (in %s): %v\n%s", strings.Join(args, " "), dir, err, out)
+	}
+	return strings.TrimSpace(string(out))
+}
+
+func initRepo(t *testing.T, dir string) {
+	t.Helper()
+	runGit(t, dir, "init", "-q", "-b", "main")
+	runGit(t, dir, "config", "user.email", "test@example.invalid")
+	runGit(t, dir, "config", "user.name", "Test User")
+	runGit(t, dir, "config", "commit.gpgsign", "false")
+}
+
+func writeRepoFile(t *testing.T, dir, name, content string) {
+	t.Helper()
+	path := filepath.Join(dir, name)
+	if err := os.MkdirAll(filepath.Dir(path), 0o755); err != nil {
+		t.Fatalf("mkdir for %s: %v", name, err)
+	}
+	if err := os.WriteFile(path, []byte(content), 0o644); err != nil {
+		t.Fatalf("write %s: %v", name, err)
+	}
+}
+
+func commitAll(t *testing.T, dir, message string) string {
+	t.Helper()
+	runGit(t, dir, "add", "-A")
+	runGit(t, dir, "commit", "-q", "-m", message)
+	return runGit(t, dir, "rev-parse", "HEAD")
+}
+
+func runApp(t *testing.T, repoDir, baseSHA, headSHA, approvalSHA string) (*OutputData, []*gh.CurrentApproval, string) {
+	t.Helper()
+
+	warnings := &bytes.Buffer{}
+	info := &bytes.Buffer{}
+
+	realClient, err := gh.NewClient("test-owner", "test-repo", "test-token")
+	if err != nil {
+		t.Fatalf("failed to build the real client: %v", err)
+	}
+	realClient.SetWarningBuffer(warnings)
+	realClient.SetInfoBuffer(info)
+
+	client := &realCheckApprovalsClient{
+		mockGitHubClient: &mockGitHubClient{
+			pr: &github.PullRequest{
+				Number: github.Ptr(1),
+				Base:   &github.PullRequestBranch{SHA: github.Ptr(baseSHA)},
+				Head:   &github.PullRequestBranch{SHA: github.Ptr(headSHA)},
+				User:   &github.User{Login: github.Ptr("author")},
+			},
+			currentApprovals: []*gh.CurrentApproval{{
+				GHLogin:   codeowners.NewSlug("@reviewer"),
+				ReviewID:  1,
+				Reviewers: []codeowners.Slug{codeowners.NewSlug("@owner")},
+				CommitID:  approvalSHA,
+			}},
+		},
+		real: realClient,
+	}
+
+	app := &App{
+		config: &Config{
+			RepoDir:       repoDir,
+			PR:            1,
+			Quiet:         true,
+			InfoBuffer:    info,
+			WarningBuffer: warnings,
+		},
+		client: client,
+	}
+
+	output, err := app.Run()
+	if err != nil {
+		t.Fatalf("app.Run failed: %v\nwarnings: %s", err, warnings)
+	}
+	return output, client.dismissed, warnings.String()
+}
+
+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
+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/config/config.go

@@ -22,6 +22,10 @@ type Config struct {
 	AllowSelfApproval           bool         `toml:"allow_self_approval"`
 	SelfApprovalViaTeams        bool         `toml:"self_approval_via_teams"`
 	DisableReviewStatusComments bool         `toml:"disable_review_status_comments"`
+	// A rebase or force-push can leave the commit an approval points at
+	// unreachable locally. Off by default because it is the only setting here
+	// which adds a network call to a run.
+	FetchOrphanedApproval bool `toml:"fetch_orphaned_approval"`
 }
 
 type Enforcement struct {