diff --git a/README.md b/README.md index 90857ad..6402f10 100644 --- a/README.md +++ b/README.md @@ -261,6 +261,10 @@ disable_review_status_comments = false # `admin_bypass` allows repository administrators to bypass codeowner requirements [admin_bypass] # see "Admin Bypass" below for more details + +# `approval_retention` allows you to specify which kinds of changes may keep an existing approval +[approval_retention] +# see "Approval Retention" below for more details ``` When a PR has any of the `high_priority_labels`, the comment will look like this: @@ -330,6 +334,38 @@ Codeowners Plus automatically detects and validates the bypass approval, immedia The bypass text is case-insensitive, so "codeowners bypass", "Codeowners Bypass", or "CODEOWNERS BYPASS" all work. +#### Approval Retention + +The `approval_retention` section lists the kinds of changes which may keep an existing approval instead of dismissing it. `enabled` is an umbrella switch for the whole section: when it is off, nothing in the section applies. + +`codeowners.toml`: +```toml +[approval_retention] +# `enabled` (default false) is the umbrella switch for the whole section +enabled = true +# `whitespace` (default follows `enabled`) retains approvals across whitespace-only changes +whitespace = true +# `comments` (default follows `enabled`) retains approvals across comment-only changes +comments = true +# `formatting` (default follows `enabled`) retains approvals across formatting-only changes +formatting = true +# `string_literals` (default false) retains approvals across string literal changes +string_literals = false +# `renames` (default false) retains approvals across renames +renames = false +# `fetch_orphaned_approval` (default false) looks for approvals which are no longer +# attached to the current commit +fetch_orphaned_approval = false +``` + +Each flag may be left unset, set to `true`, or set to `false`, and an explicit value always wins over the umbrella: + +- Umbrella off, nothing else set: every flag is off +- Umbrella on, nothing else set: every flag which follows the umbrella is on +- Umbrella on with `comments = false`: every flag which follows the umbrella is on except `comments` + +`string_literals`, `renames` and `fetch_orphaned_approval` are opt-in only - the umbrella never turns them on by itself, and they must be set to `true` explicitly. 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. `fetch_orphaned_approval` is the odd one out: it is opt-in because it is the only flag in the section which reaches outside the checkout, and enabling the umbrella should not quietly add network calls to a run. + #### 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. diff --git a/internal/app/approval_retention_test.go b/internal/app/approval_retention_test.go new file mode 100644 index 0000000..45a6f68 --- /dev/null +++ b/internal/app/approval_retention_test.go @@ -0,0 +1,224 @@ +package app + +import ( + "bytes" + "os" + "os/exec" + "path/filepath" + "slices" + "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 retentionBaseSource = `package service + +func Alpha() int { + return 1 +} + +func Beta() int { + return 2 +} +` + +// retentionApprovedSource is the change the reviewer approved. +const retentionApprovedSource = `package service + +func Alpha() int { + return 1 +} + +func Beta() int { + return 20 +} +` + +// Adds a comment and nothing else, so a comment is all the reviewer has not seen. +const retentionHeadSource = `package service + +// Alpha is the first step. +func Alpha() int { + return 1 +} + +func Beta() int { + return 20 +} +` + +// configBody is committed as codeowners.toml on the base ref, which is where the +// application reads its configuration from. +func buildCommentOnlyRepo(t *testing.T, configBody string) (repoDir, baseSHA, headSHA, approvalSHA string) { + t.Helper() + repoDir = t.TempDir() + initRepo(t, repoDir) + + writeRepoFile(t, repoDir, ".codeowners", "* @owner\n") + writeRepoFile(t, repoDir, "codeowners.toml", configBody) + writeRepoFile(t, repoDir, "service.go", retentionBaseSource) + baseSHA = commitAll(t, repoDir, "base") + + writeRepoFile(t, repoDir, "service.go", retentionApprovedSource) + approvalSHA = commitAll(t, repoDir, "approved change") + + writeRepoFile(t, repoDir, "service.go", retentionHeadSource) + headSHA = commitAll(t, repoDir, "comment on top of the approved change") + + return repoDir, baseSHA, headSHA, approvalSHA +} + +const retentionOffConfig = `disable_review_status_comments = true +` + +// The feature is inert until asked for: no section and an all-off section have to +// produce the same bytes. +func TestRunWithoutRetentionSectionIsUnchanged(t *testing.T) { + const explicitlyOff = `disable_review_status_comments = true + +[approval_retention] +enabled = false +whitespace = false +comments = false +formatting = false +string_literals = false +renames = false +fetch_orphaned_approval = false +` + + repoDir, baseSHA, headSHA, approvalSHA := buildCommentOnlyRepo(t, retentionOffConfig) + absentOutput, absentDismissed, absentWarnings := runApp(t, repoDir, baseSHA, headSHA, approvalSHA) + + repoDir, baseSHA, headSHA, approvalSHA = buildCommentOnlyRepo(t, explicitlyOff) + offOutput, offDismissed, offWarnings := runApp(t, repoDir, baseSHA, headSHA, approvalSHA) + + if absentOutput.Message != offOutput.Message || absentOutput.Success != offOutput.Success { + t.Errorf("expected identical results, got %+v and %+v", absentOutput, offOutput) + } + if !slices.Equal(absentOutput.StillRequired, offOutput.StillRequired) { + t.Errorf("expected identical still required, got %v and %v", absentOutput.StillRequired, offOutput.StillRequired) + } + if len(absentDismissed) != len(offDismissed) { + t.Errorf("expected identical dismissals, got %d and %d", len(absentDismissed), len(offDismissed)) + } + if absentWarnings != offWarnings { + t.Errorf("expected identical warnings, got %q and %q", absentWarnings, offWarnings) + } + // Both are the pre-feature behavior, not merely equal to each other. + if len(absentDismissed) != 1 || absentOutput.Success { + t.Errorf("expected the approval to be dismissed as it always was, got %d dismissals, success %t", + len(absentDismissed), absentOutput.Success) + } +} diff --git a/internal/config/config.go b/internal/config/config.go index c3bcf7e..fc7cbd8 100644 --- a/internal/config/config.go +++ b/internal/config/config.go @@ -8,20 +8,21 @@ import ( ) type Config struct { - MaxReviews *int `toml:"max_reviews"` - MinReviews *int `toml:"min_reviews"` - UnskippableReviewers []string `toml:"unskippable_reviewers"` - Ignore []string `toml:"ignore"` - Enforcement *Enforcement `toml:"enforcement"` - HighPriorityLabels []string `toml:"high_priority_labels"` - AdminBypass *AdminBypass `toml:"admin_bypass"` - DetailedReviewers bool `toml:"detailed_reviewers"` - DisableSmartDismissal bool `toml:"disable_smart_dismissal"` - RequireBothBranchReviewers bool `toml:"require_both_branch_reviewers"` - SuppressUnownedWarning bool `toml:"suppress_unowned_warning"` - AllowSelfApproval bool `toml:"allow_self_approval"` - SelfApprovalViaTeams bool `toml:"self_approval_via_teams"` - DisableReviewStatusComments bool `toml:"disable_review_status_comments"` + MaxReviews *int `toml:"max_reviews"` + MinReviews *int `toml:"min_reviews"` + UnskippableReviewers []string `toml:"unskippable_reviewers"` + Ignore []string `toml:"ignore"` + Enforcement *Enforcement `toml:"enforcement"` + HighPriorityLabels []string `toml:"high_priority_labels"` + AdminBypass *AdminBypass `toml:"admin_bypass"` + ApprovalRetention *ApprovalRetention `toml:"approval_retention"` + DetailedReviewers bool `toml:"detailed_reviewers"` + DisableSmartDismissal bool `toml:"disable_smart_dismissal"` + RequireBothBranchReviewers bool `toml:"require_both_branch_reviewers"` + SuppressUnownedWarning bool `toml:"suppress_unowned_warning"` + AllowSelfApproval bool `toml:"allow_self_approval"` + SelfApprovalViaTeams bool `toml:"self_approval_via_teams"` + DisableReviewStatusComments bool `toml:"disable_review_status_comments"` } type Enforcement struct { @@ -34,6 +35,74 @@ type AdminBypass struct { AllowedUsers []string `toml:"allowed_users"` } +// ApprovalRetention lists the kinds of diff change which may retain an approval. +// The flags are *bool so that unset can be told apart from an explicit false. +type ApprovalRetention struct { + Enabled bool `toml:"enabled"` + Whitespace *bool `toml:"whitespace"` + Comments *bool `toml:"comments"` + Formatting *bool `toml:"formatting"` + StringLiterals *bool `toml:"string_literals"` + Renames *bool `toml:"renames"` + FetchOrphanedApproval *bool `toml:"fetch_orphaned_approval"` +} + +func (r *ApprovalRetention) WhitespaceEnabled() bool { + if r == nil { + return false + } + return r.defaultOn(r.Whitespace) +} + +func (r *ApprovalRetention) CommentsEnabled() bool { + if r == nil { + return false + } + return r.defaultOn(r.Comments) +} + +func (r *ApprovalRetention) FormattingEnabled() bool { + if r == nil { + return false + } + return r.defaultOn(r.Formatting) +} + +func (r *ApprovalRetention) StringLiteralsEnabled() bool { + if r == nil { + return false + } + return r.defaultOff(r.StringLiterals) +} + +func (r *ApprovalRetention) RenamesEnabled() bool { + if r == nil { + return false + } + return r.defaultOff(r.Renames) +} + +func (r *ApprovalRetention) FetchOrphanedApprovalEnabled() bool { + if r == nil { + return false + } + return r.defaultOff(r.FetchOrphanedApproval) +} + +func (r *ApprovalRetention) defaultOn(flag *bool) bool { + if !r.Enabled { + return false + } + if flag == nil { + return true + } + return *flag +} + +func (r *ApprovalRetention) defaultOff(flag *bool) bool { + return r.Enabled && flag != nil && *flag +} + func ReadConfig(path string, fileReader codeowners.FileReader) (*Config, error) { if !strings.HasSuffix(path, "/") { path += "/" @@ -47,6 +116,7 @@ func ReadConfig(path string, fileReader codeowners.FileReader) (*Config, error) Enforcement: &Enforcement{Approval: false, FailCheck: true}, HighPriorityLabels: []string{}, AdminBypass: &AdminBypass{Enabled: false, AllowedUsers: []string{}}, + ApprovalRetention: &ApprovalRetention{Enabled: false}, DetailedReviewers: false, SelfApprovalViaTeams: false, DisableSmartDismissal: false, @@ -79,5 +149,8 @@ func ReadConfig(path string, fileReader codeowners.FileReader) (*Config, error) if config.AdminBypass == nil { config.AdminBypass = defaultConfig.AdminBypass } + if config.ApprovalRetention == nil { + config.ApprovalRetention = defaultConfig.ApprovalRetention + } return config, nil } diff --git a/internal/config/config_test.go b/internal/config/config_test.go index f09c117..a00c465 100644 --- a/internal/config/config_test.go +++ b/internal/config/config_test.go @@ -250,6 +250,213 @@ func TestReadConfigFileError(t *testing.T) { } } +func TestApprovalRetention(t *testing.T) { + type resolved struct { + whitespace bool + comments bool + formatting bool + stringLiterals bool + renames bool + fetchOrphanedApproval bool + } + + tt := []struct { + name string + configContent string + expected resolved + }{ + { + name: "section absent", + configContent: "max_reviews = 2", + expected: resolved{}, + }, + { + name: "umbrella off with nothing else set", + configContent: ` +[approval_retention] +enabled = false +`, + expected: resolved{}, + }, + { + name: "umbrella off ignores explicitly enabled flags", + configContent: ` +[approval_retention] +enabled = false +whitespace = true +string_literals = true +renames = true +fetch_orphaned_approval = true +`, + expected: resolved{}, + }, + { + name: "umbrella on with nothing else set", + configContent: ` +[approval_retention] +enabled = true +`, + expected: resolved{ + whitespace: true, + comments: true, + formatting: true, + }, + }, + // Each case turns exactly one flag off (or on, for the opt-ins), so an + // accessor reading the wrong struct field fails one of them. + { + name: "umbrella on with only comments off", + configContent: ` +[approval_retention] +enabled = true +comments = false +`, + expected: resolved{ + whitespace: true, + formatting: true, + }, + }, + { + name: "umbrella on with only whitespace off", + configContent: ` +[approval_retention] +enabled = true +whitespace = false +`, + expected: resolved{ + comments: true, + formatting: true, + }, + }, + { + name: "umbrella on with only formatting off", + configContent: ` +[approval_retention] +enabled = true +formatting = false +`, + expected: resolved{ + whitespace: true, + comments: true, + }, + }, + { + name: "umbrella on with only string_literals opted in", + configContent: ` +[approval_retention] +enabled = true +string_literals = true +`, + expected: resolved{ + whitespace: true, + comments: true, + formatting: true, + stringLiterals: true, + }, + }, + { + name: "umbrella on with only renames opted in", + configContent: ` +[approval_retention] +enabled = true +renames = true +`, + expected: resolved{ + whitespace: true, + comments: true, + formatting: true, + renames: true, + }, + }, + { + name: "umbrella on with only fetch_orphaned_approval opted in", + configContent: ` +[approval_retention] +enabled = true +fetch_orphaned_approval = true +`, + expected: resolved{ + whitespace: true, + comments: true, + formatting: true, + fetchOrphanedApproval: true, + }, + }, + { + name: "umbrella on with every flag explicitly false", + configContent: ` +[approval_retention] +enabled = true +whitespace = false +comments = false +formatting = false +string_literals = false +renames = false +fetch_orphaned_approval = false +`, + expected: resolved{}, + }, + { + name: "opt-in only flags require being set explicitly", + configContent: ` +[approval_retention] +enabled = true +string_literals = true +renames = true +fetch_orphaned_approval = true +`, + expected: resolved{ + whitespace: true, + comments: true, + formatting: true, + stringLiterals: true, + renames: true, + fetchOrphanedApproval: true, + }, + }, + } + + for _, tc := range tt { + t.Run(tc.name, func(t *testing.T) { + testDir := t.TempDir() + err := os.WriteFile(filepath.Join(testDir, "codeowners.toml"), []byte(tc.configContent), 0644) + if err != nil { + t.Fatalf("failed to write test config: %v", err) + } + + config, err := ReadConfig(testDir, nil) + if err != nil { + t.Fatalf("unexpected error: %v", err) + } + + if config.ApprovalRetention == nil { + t.Fatal("expected ApprovalRetention to be set") + } + + got := resolved{ + whitespace: config.ApprovalRetention.WhitespaceEnabled(), + comments: config.ApprovalRetention.CommentsEnabled(), + formatting: config.ApprovalRetention.FormattingEnabled(), + stringLiterals: config.ApprovalRetention.StringLiteralsEnabled(), + renames: config.ApprovalRetention.RenamesEnabled(), + fetchOrphanedApproval: config.ApprovalRetention.FetchOrphanedApprovalEnabled(), + } + if got != tc.expected { + t.Errorf("resolved flags: expected %+v, got %+v", tc.expected, got) + } + }) + } +} + +func TestApprovalRetentionNilSection(t *testing.T) { + var retention *ApprovalRetention + + if retention.WhitespaceEnabled() || retention.CommentsEnabled() || retention.FormattingEnabled() || + retention.StringLiteralsEnabled() || retention.RenamesEnabled() || retention.FetchOrphanedApprovalEnabled() { + t.Error("expected all flags to be disabled for a nil section") + } +} + // Helper functions func intPtr(i int) *int { return &i diff --git a/internal/git/diff_test.go b/internal/git/diff_test.go index 59a2244..729e1ad 100644 --- a/internal/git/diff_test.go +++ b/internal/git/diff_test.go @@ -129,7 +129,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, }, },