From 5a2668352bcf6fd797cd78ded3191699a25e87a9 Mon Sep 17 00:00:00 2001 From: Ezekiel Lopez Date: Tue, 1 Sep 2026 00:08:29 -0700 Subject: [PATCH 1/3] 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...`, 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 ref is probed with `cat-file` first, so a diff which failed for some other reason does not spend a remote round trip that cannot help. - 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. Coverage badge regenerated. --- README.md | 9 +- internal/app/app.go | 6 +- internal/app/orphaned_approval_test.go | 214 ++++++++++++++++++++++ internal/config/config.go | 1 + internal/git/diff.go | 80 ++++++++- internal/git/diff_test.go | 239 ++++++++++++++++++++++++- 6 files changed, 537 insertions(+), 12 deletions(-) create mode 100644 internal/app/orphaned_approval_test.go diff --git a/README.md b/README.md index 90857ad..aa4db70 100644 --- a/README.md +++ b/README.md @@ -4,7 +4,7 @@ Code Ownership & Review Assignment Tool - GitHub CODEOWNERS but better [![Go Report Card](https://goreportcard.com/badge/github.com/multimediallc/codeowners-plus)](https://goreportcard.com/report/github.com/multimediallc/codeowners-plus?kill_cache=1) [![Tests](https://github.com/multimediallc/codeowners-plus/actions/workflows/go.yml/badge.svg)](https://github.com/multimediallc/codeowners-plus/actions/workflows/go.yml) -![Coverage](https://img.shields.io/badge/Coverage-82.6%25-brightgreen) +![Coverage](https://img.shields.io/badge/Coverage-85.2%25-brightgreen) [![License](https://img.shields.io/badge/License-BSD%203--Clause-blue.svg)](https://opensource.org/licenses/BSD-3-Clause) [![Contributor Covenant](https://img.shields.io/badge/Contributor%20Covenant-2.1-4baaaa.svg)](CODE_OF_CONDUCT.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 diff --git a/internal/app/app.go b/internal/app/app.go index d962024..2b01d12 100644 --- a/internal/app/app.go +++ b/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) } diff --git a/internal/app/orphaned_approval_test.go b/internal/app/orphaned_approval_test.go new file mode 100644 index 0000000..9c7dc9c --- /dev/null +++ b/internal/app/orphaned_approval_test.go @@ -0,0 +1,214 @@ +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" +) + +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 +} +` + +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") + + 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 +} + +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) + + 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) + } + }) + } +} diff --git a/internal/config/config.go b/internal/config/config.go index c3bcf7e..a0f0962 100644 --- a/internal/config/config.go +++ b/internal/config/config.go @@ -22,6 +22,7 @@ type Config struct { AllowSelfApproval bool `toml:"allow_self_approval"` SelfApprovalViaTeams bool `toml:"self_approval_via_teams"` DisableReviewStatusComments bool `toml:"disable_review_status_comments"` + FetchOrphanedApproval bool `toml:"fetch_orphaned_approval"` } type Enforcement struct { diff --git a/internal/git/diff.go b/internal/git/diff.go index 95c74c0..1db58c2 100644 --- a/internal/git/diff.go +++ b/internal/git/diff.go @@ -3,21 +3,30 @@ 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" ) +const fetchTimeout = 60 * time.Second + // gitCommandExecutor defines the interface for executing git commands type gitCommandExecutor interface { execute(command string, args ...string) ([]byte, error) } +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,6 +42,18 @@ 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) { + 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) @@ -40,18 +61,29 @@ type Diff interface { } 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. +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 +93,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 +117,15 @@ func (gd *GitDiff) ChangesSince(ref string) ([]codeowners.DiffFile, error) { IgnoreDirs: gd.context.IgnoreDirs, } olderDiff, err := getGitDiff(olderDiffContext, gd.executor) + if err != nil && gd.fetchOrphanedRefs && !gd.refResolvesLocally(ref) { + 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 +140,23 @@ func (gd *GitDiff) ChangesSince(ref string) ([]codeowners.DiffFile, error) { return diffFiles, nil } +// A diff can fail for reasons unrelated to the ref, and fetching then cannot help. +func (gd *GitDiff) refResolvesLocally(ref string) bool { + _, err := gd.executor.execute("git", "cat-file", "-e", ref+"^{commit}") + return err == nil +} + +// `git fetch` accepts --upload-pack, which names a command to run, so the ref goes after a `--`. +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 } diff --git a/internal/git/diff_test.go b/internal/git/diff_test.go index 59a2244..a883ac7 100644 --- a/internal/git/diff_test.go +++ b/internal/git/diff_test.go @@ -5,7 +5,11 @@ import ( "fmt" "io" "os" + "os/exec" + "slices" + "strings" "testing" + "time" "github.com/multimediallc/codeowners-plus/pkg/codeowners" "github.com/sourcegraph/go-diff/diff" @@ -31,6 +35,59 @@ func (e *mockGitExecutor) execute(command string, args ...string) ([]byte, error return []byte(e.output), nil } +type mockResult struct { + output string + err error +} + +type scriptedGitExecutor struct { + diffResults []mockResult + fetchResult mockResult + refResolves bool + calls [][]string +} + +func (e *scriptedGitExecutor) execute(command string, args ...string) ([]byte, error) { + e.calls = append(e.calls, append([]string{command}, args...)) + res := e.fetchResult + if len(args) > 0 && args[0] == "cat-file" { + if e.refResolves { + return nil, nil + } + return nil, errors.New("not a valid object name") + } + if len(args) > 0 && args[0] == "diff" { + if len(e.diffResults) == 0 { + return nil, errors.New("unexpected git diff call") + } + res, e.diffResults = e.diffResults[0], e.diffResults[1:] + } + if res.err != nil { + return nil, res.err + } + return []byte(res.output), nil +} + +type timeoutScriptedExecutor struct { + scriptedGitExecutor + timeouts []time.Duration +} + +func (e *timeoutScriptedExecutor) executeWithTimeout(timeout time.Duration, command string, args ...string) ([]byte, error) { + e.timeouts = append(e.timeouts, timeout) + return e.execute(command, args...) +} + +func (e *scriptedGitExecutor) fetchCalls() [][]string { + fetches := make([][]string, 0, len(e.calls)) + for _, call := range e.calls { + if len(call) > 1 && call[1] == "fetch" { + fetches = append(fetches, call) + } + } + return fetches +} + func readFile(path string) ([]byte, error) { file, err := os.Open(path) if err != nil { @@ -129,7 +186,7 @@ Binary files a/assets/img/offline.png and b/assets/img/offline.png differ`, expectedErr: false, expectedFiles: 2, expectedHunks: map[string]int{ - "file1.go": 1, + "file1.go": 1, "assets/img/offline.png": 0, }, }, @@ -354,6 +411,186 @@ index abc..def 100644 } } +func TestChangesSinceFetchOrphanedRef(t *testing.T) { + const olderDiff = `diff --git a/file1.go b/file1.go +index abc..def 100644 +--- a/file1.go ++++ b/file1.go +@@ -5,0 +6 @@ func Example() { ++ fmt.Println("Old change")` + + diffFailure := errors.New("fatal: bad object deadbeef") + retryFailure := errors.New("fatal: ambiguous argument 'orphaned-ref'") + fetchFailure := errors.New("fatal: could not read from remote") + + tt := []struct { + name string + fetchOrphanedRefs bool + refResolves bool + olderDiffResults []mockResult + fetchResult mockResult + expectedErr error // the error the caller must see as the cause + alsoInErr error // secondary error which must stay visible + expectedFetchCalls int + expectedFiles int + }{ + { + name: "older diff succeeds, no fetch attempted", + fetchOrphanedRefs: true, + olderDiffResults: []mockResult{{output: olderDiff}}, + expectedFetchCalls: 0, + expectedFiles: 2, + }, + { + name: "fetch recovers the orphaned ref", + fetchOrphanedRefs: true, + olderDiffResults: []mockResult{{err: diffFailure}, {output: olderDiff}}, + expectedFetchCalls: 1, + expectedFiles: 2, + }, + { + name: "fetch fails", + fetchOrphanedRefs: true, + olderDiffResults: []mockResult{{err: diffFailure}}, + fetchResult: mockResult{err: fetchFailure}, + expectedErr: diffFailure, + alsoInErr: fetchFailure, + expectedFetchCalls: 1, + }, + { + name: "retry after fetch still fails", + fetchOrphanedRefs: true, + olderDiffResults: []mockResult{{err: diffFailure}, {err: retryFailure}}, + expectedErr: diffFailure, + alsoInErr: retryFailure, + expectedFetchCalls: 1, + }, + { + name: "disabled, no fetch attempted", + fetchOrphanedRefs: false, + olderDiffResults: []mockResult{{err: diffFailure}}, + expectedErr: diffFailure, + expectedFetchCalls: 0, + }, + { + name: "ref resolves locally, so the diff failed for another reason", + fetchOrphanedRefs: true, + refResolves: true, + olderDiffResults: []mockResult{{err: diffFailure}}, + expectedErr: diffFailure, + expectedFetchCalls: 0, + }, + } + + for _, tc := range tt { + t.Run(tc.name, func(t *testing.T) { + executor := &scriptedGitExecutor{ + diffResults: append([]mockResult{{output: sampleGitDiff}}, tc.olderDiffResults...), + fetchResult: tc.fetchResult, + refResolves: tc.refResolves, + } + + var opts []DiffOption + if tc.fetchOrphanedRefs { + opts = append(opts, WithFetchOrphanedRefs()) + } + context := DiffContext{Base: "main", Head: "feature", Dir: "."} + diff, err := NewDiffWithExecutor(context, executor, opts...) + if err != nil { + t.Fatalf("failed to create initial diff: %v", err) + } + + changes, err := diff.ChangesSince("orphaned-ref") + + fetches := executor.fetchCalls() + if len(fetches) != tc.expectedFetchCalls { + t.Errorf("expected %d fetch calls, got %d", tc.expectedFetchCalls, len(fetches)) + } + for _, fetch := range fetches { + want := []string{"git", "fetch", "--no-tags", "origin", "--", "orphaned-ref"} + if !slices.Equal(fetch, want) { + t.Errorf("expected fetch command %v, got %v", want, fetch) + } + } + + if tc.expectedErr != nil { + if err == nil { + t.Fatal("expected error but got none") + } + wantPrefix := "failed to get older diff: diff Error: " + tc.expectedErr.Error() + if !strings.HasPrefix(err.Error(), wantPrefix) { + t.Errorf("expected error to start with %q, got %v", wantPrefix, err) + } + if tc.alsoInErr != nil && !strings.Contains(err.Error(), tc.alsoInErr.Error()) { + t.Errorf("expected %q to stay visible, got %v", tc.alsoInErr, err) + } + return + } + + if err != nil { + t.Fatalf("unexpected error: %v", err) + } + if len(changes) != tc.expectedFiles { + t.Errorf("expected %d files, got %d", tc.expectedFiles, len(changes)) + } + }) + } +} + +func TestChangesSinceFetchIsBounded(t *testing.T) { + const olderDiff = `diff --git a/file1.go b/file1.go +index abc..def 100644 +--- a/file1.go ++++ b/file1.go +@@ -5,0 +6 @@ func Example() { ++ fmt.Println("Old change")` + + executor := &timeoutScriptedExecutor{ + scriptedGitExecutor: scriptedGitExecutor{ + diffResults: []mockResult{ + {output: sampleGitDiff}, + {err: errors.New("fatal: bad object deadbeef")}, + {output: olderDiff}, + }, + }, + } + + context := DiffContext{Base: "main", Head: "feature", Dir: "."} + diff, err := NewDiffWithExecutor(context, executor, WithFetchOrphanedRefs()) + if err != nil { + t.Fatalf("failed to create initial diff: %v", err) + } + if _, err := diff.ChangesSince("orphaned-ref"); err != nil { + t.Fatalf("unexpected error: %v", err) + } + + if len(executor.timeouts) != 1 { + t.Fatalf("expected 1 bounded command, got %d", len(executor.timeouts)) + } + if executor.timeouts[0] != fetchTimeout { + t.Errorf("expected the fetch to be bounded by %s, got %s", fetchTimeout, executor.timeouts[0]) + } +} + +func TestExecuteWithTimeout(t *testing.T) { + if _, err := exec.LookPath("sleep"); err != nil { + t.Skip("sleep not available") + } + executor := newRealGitExecutor(".") + + if _, err := executor.executeWithTimeout(time.Minute, "sleep", "0"); err != nil { + t.Errorf("unexpected error for a command within the timeout: %v", err) + } + + _, err := executor.executeWithTimeout(10*time.Millisecond, "sleep", "30") + if err == nil { + t.Fatal("expected an error when the command outlives the timeout") + } + if !strings.Contains(err.Error(), "timed out") { + t.Errorf("expected a timeout error, got %v", err) + } +} + func TestHunkHash(t *testing.T) { tt := []struct { name string From 2288fd6faa995b073198f987684cd7b7a5316cd6 Mon Sep 17 00:00:00 2001 From: Ezekiel Lopez Date: Tue, 1 Sep 2026 12:30:09 -0700 Subject: [PATCH 2/3] fix: surface git's own reason when the fetch fails Review catch: fetchRef discarded CombinedOutput and returned only the error, so a failed fetch reached the caller as a bare "exit status 128" with nothing about why. getGitDiff already wraps its output, so this was inconsistent with the rest of the file. The scripted test executor had the same bug, returning nil output alongside an error, which is why nothing caught this. It now returns both, the way CombinedOutput does. --- internal/git/diff.go | 14 ++++++++++---- internal/git/diff_test.go | 25 +++++++++++++++++++++---- 2 files changed, 31 insertions(+), 8 deletions(-) diff --git a/internal/git/diff.go b/internal/git/diff.go index 1db58c2..75b79cc 100644 --- a/internal/git/diff.go +++ b/internal/git/diff.go @@ -149,12 +149,18 @@ func (gd *GitDiff) refResolvesLocally(ref string) bool { // `git fetch` accepts --upload-pack, which names a command to run, so the ref goes after a `--`. func (gd *GitDiff) fetchRef(ref string) error { args := []string{"fetch", "--no-tags", "origin", "--", ref} + run := gd.executor.execute if executor, ok := gd.executor.(timeoutExecutor); ok { - _, err := executor.executeWithTimeout(fetchTimeout, "git", args...) - return err + run = func(command string, args ...string) ([]byte, error) { + return executor.executeWithTimeout(fetchTimeout, command, args...) + } + } + // git puts the reason on stderr and the exit status alone says nothing useful, + // which is why getGitDiff wraps its output too. + if output, err := run("git", args...); err != nil { + return fmt.Errorf("%s\n%s", err, output) } - _, err := gd.executor.execute("git", args...) - return err + return nil } func (gd *GitDiff) Context() DiffContext { diff --git a/internal/git/diff_test.go b/internal/git/diff_test.go index a883ac7..389a572 100644 --- a/internal/git/diff_test.go +++ b/internal/git/diff_test.go @@ -62,10 +62,8 @@ func (e *scriptedGitExecutor) execute(command string, args ...string) ([]byte, e } res, e.diffResults = e.diffResults[0], e.diffResults[1:] } - if res.err != nil { - return nil, res.err - } - return []byte(res.output), nil + // CombinedOutput returns both, and the output is where git puts the reason. + return []byte(res.output), res.err } type timeoutScriptedExecutor struct { @@ -411,6 +409,25 @@ index abc..def 100644 } } +func TestFetchRefSurfacesGitOutput(t *testing.T) { + executor := &scriptedGitExecutor{ + diffResults: []mockResult{{output: sampleGitDiff}, {err: errors.New("fatal: bad object deadbeef")}}, + fetchResult: mockResult{output: "fatal: remote error: upload-pack not permitted", err: errors.New("exit status 128")}, + } + diff, err := NewDiffWithExecutor(DiffContext{Base: "main", Head: "feature", Dir: "."}, executor, WithFetchOrphanedRefs()) + if err != nil { + t.Fatalf("failed to create initial diff: %v", err) + } + + _, err = diff.ChangesSince("orphaned-ref") + if err == nil { + t.Fatal("expected an error") + } + if !strings.Contains(err.Error(), "upload-pack not permitted") { + t.Errorf("expected git's own reason to reach the caller, got %v", err) + } +} + func TestChangesSinceFetchOrphanedRef(t *testing.T) { const olderDiff = `diff --git a/file1.go b/file1.go index abc..def 100644 From 7c76ea848a593b4a1439287d9d81779429de7404 Mon Sep 17 00:00:00 2001 From: Ezekiel Lopez Date: Tue, 1 Sep 2026 13:55:41 -0700 Subject: [PATCH 3/3] fix: actually bound the fetch, and only fetch an object name Review catch, and the same defect the hunk-filter runner already guards against. exec.CommandContext SIGKILLs git, but `git fetch` spawns git-remote-https, which inherits the CombinedOutput pipe, and Wait blocks until every writer closes. So against an unresponsive remote the call sat there long past the deadline while the error claimed it had timed out. cmd.WaitDelay bounds the wait for the helper to go away. The ref is also now required to look like an object name before it reaches the network. `--` already stopped a dash-leading ref being read as an option, but it does not stop refspec parsing, and `refs/heads/main:refs/heads/injected` created a local ref. The only caller passes a GitHub-issued SHA, so this is defence in depth, and it closes the empty-ref case for free: git reads an empty ref as HEAD, which would have diffed base...HEAD and let an approval pass ownership on the wrong comparison. Test refs are object names now, since that is what production passes, plus rows for a refspec-shaped ref and an empty one. README: `fetch_orphaned_approval` was described as the only setting that adds a network call, which is wrong because self_approval_via_teams fetches team members, and the 60s bound was not true until the WaitDelay fix above. --- README.md | 7 ++++--- internal/git/diff.go | 18 +++++++++++++++++- internal/git/diff_test.go | 31 ++++++++++++++++++++++++++----- 3 files changed, 47 insertions(+), 9 deletions(-) diff --git a/README.md b/README.md index aa4db70..34cbae7 100644 --- a/README.md +++ b/README.md @@ -4,7 +4,7 @@ Code Ownership & Review Assignment Tool - GitHub CODEOWNERS but better [![Go Report Card](https://goreportcard.com/badge/github.com/multimediallc/codeowners-plus)](https://goreportcard.com/report/github.com/multimediallc/codeowners-plus?kill_cache=1) [![Tests](https://github.com/multimediallc/codeowners-plus/actions/workflows/go.yml/badge.svg)](https://github.com/multimediallc/codeowners-plus/actions/workflows/go.yml) -![Coverage](https://img.shields.io/badge/Coverage-85.2%25-brightgreen) +![Coverage](https://img.shields.io/badge/Coverage-85.3%25-brightgreen) [![License](https://img.shields.io/badge/License-BSD%203--Clause-blue.svg)](https://opensource.org/licenses/BSD-3-Clause) [![Contributor Covenant](https://img.shields.io/badge/Contributor%20Covenant-2.1-4baaaa.svg)](CODE_OF_CONDUCT.md) @@ -257,8 +257,9 @@ 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. +# diff it would be judged on cannot be computed. Off by default because it reaches +# the network: one fetch per unresolvable commit, bounded at 60s plus a short grace +# for the transport helper to exit. fetch_orphaned_approval = false # `enforcement` allows you to specify how the Codeowners Plus check should be enforced diff --git a/internal/git/diff.go b/internal/git/diff.go index 75b79cc..6771239 100644 --- a/internal/git/diff.go +++ b/internal/git/diff.go @@ -18,6 +18,9 @@ import ( const fetchTimeout = 60 * time.Second +// git spawns a transport helper which inherits the pipe and outlives the kill. +const transportHelperGrace = 2 * time.Second + // gitCommandExecutor defines the interface for executing git commands type gitCommandExecutor interface { execute(command string, args ...string) ([]byte, error) @@ -47,6 +50,7 @@ func (e *realGitExecutor) executeWithTimeout(timeout time.Duration, command stri defer cancel() cmd := exec.CommandContext(ctx, command, args...) cmd.Dir = e.dir + cmd.WaitDelay = transportHelperGrace output, err := cmd.CombinedOutput() if errors.Is(ctx.Err(), context.DeadlineExceeded) { return output, fmt.Errorf("%s timed out after %s", command, timeout) @@ -117,7 +121,7 @@ func (gd *GitDiff) ChangesSince(ref string) ([]codeowners.DiffFile, error) { IgnoreDirs: gd.context.IgnoreDirs, } olderDiff, err := getGitDiff(olderDiffContext, gd.executor) - if err != nil && gd.fetchOrphanedRefs && !gd.refResolvesLocally(ref) { + if err != nil && gd.fetchOrphanedRefs && looksLikeObjectName(ref) && !gd.refResolvesLocally(ref) { 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 { @@ -140,6 +144,18 @@ func (gd *GitDiff) ChangesSince(ref string) ([]codeowners.DiffFile, error) { return diffFiles, nil } +func looksLikeObjectName(ref string) bool { + if len(ref) < 7 || len(ref) > 64 { + return false + } + for i := 0; i < len(ref); i++ { + if c := ref[i]; (c < '0' || c > '9') && (c < 'a' || c > 'f') { + return false + } + } + return true +} + // A diff can fail for reasons unrelated to the ref, and fetching then cannot help. func (gd *GitDiff) refResolvesLocally(ref string) bool { _, err := gd.executor.execute("git", "cat-file", "-e", ref+"^{commit}") diff --git a/internal/git/diff_test.go b/internal/git/diff_test.go index 389a572..eb3e913 100644 --- a/internal/git/diff_test.go +++ b/internal/git/diff_test.go @@ -419,7 +419,7 @@ func TestFetchRefSurfacesGitOutput(t *testing.T) { t.Fatalf("failed to create initial diff: %v", err) } - _, err = diff.ChangesSince("orphaned-ref") + _, err = diff.ChangesSince("deadbeefdeadbeefdeadbeefdeadbeefdeadbeef") if err == nil { t.Fatal("expected an error") } @@ -437,13 +437,14 @@ index abc..def 100644 + fmt.Println("Old change")` diffFailure := errors.New("fatal: bad object deadbeef") - retryFailure := errors.New("fatal: ambiguous argument 'orphaned-ref'") + retryFailure := errors.New("fatal: ambiguous argument 'deadbeefdeadbeefdeadbeefdeadbeefdeadbeef'") fetchFailure := errors.New("fatal: could not read from remote") tt := []struct { name string fetchOrphanedRefs bool refResolves bool + ref string olderDiffResults []mockResult fetchResult mockResult expectedErr error // the error the caller must see as the cause @@ -489,6 +490,22 @@ index abc..def 100644 expectedErr: diffFailure, expectedFetchCalls: 0, }, + { + name: "ref is not an object name, so nothing is fetched", + fetchOrphanedRefs: true, + ref: "refs/heads/main:refs/heads/injected", + olderDiffResults: []mockResult{{err: diffFailure}}, + expectedErr: diffFailure, + expectedFetchCalls: 0, + }, + { + name: "empty ref is never fetched", + fetchOrphanedRefs: true, + ref: "", + olderDiffResults: []mockResult{{err: diffFailure}}, + expectedErr: diffFailure, + expectedFetchCalls: 0, + }, { name: "ref resolves locally, so the diff failed for another reason", fetchOrphanedRefs: true, @@ -517,14 +534,18 @@ index abc..def 100644 t.Fatalf("failed to create initial diff: %v", err) } - changes, err := diff.ChangesSince("orphaned-ref") + ref := tc.ref + if ref == "" && tc.name != "empty ref is never fetched" { + ref = "deadbeefdeadbeefdeadbeefdeadbeefdeadbeef" + } + changes, err := diff.ChangesSince(ref) fetches := executor.fetchCalls() if len(fetches) != tc.expectedFetchCalls { t.Errorf("expected %d fetch calls, got %d", tc.expectedFetchCalls, len(fetches)) } for _, fetch := range fetches { - want := []string{"git", "fetch", "--no-tags", "origin", "--", "orphaned-ref"} + want := []string{"git", "fetch", "--no-tags", "origin", "--", "deadbeefdeadbeefdeadbeefdeadbeefdeadbeef"} if !slices.Equal(fetch, want) { t.Errorf("expected fetch command %v, got %v", want, fetch) } @@ -577,7 +598,7 @@ index abc..def 100644 if err != nil { t.Fatalf("failed to create initial diff: %v", err) } - if _, err := diff.ChangesSince("orphaned-ref"); err != nil { + if _, err := diff.ChangesSince("deadbeefdeadbeefdeadbeefdeadbeefdeadbeef"); err != nil { t.Fatalf("unexpected error: %v", err) }