fix: return real defaults when codeowners.toml fails to parse

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-82.7%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)
 

internal/config/config.go

@@ -34,12 +34,8 @@ type AdminBypass struct {
 	AllowedUsers []string `toml:"allowed_users"`
 }
 
-func ReadConfig(path string, fileReader codeowners.FileReader) (*Config, error) {
-	if !strings.HasSuffix(path, "/") {
-		path += "/"
-	}
-
-	defaultConfig := &Config{
+func newDefaultConfig() *Config {
+	return &Config{
 		MaxReviews:                  nil,
 		MinReviews:                  nil,
 		UnskippableReviewers:        []string{},
@@ -51,8 +47,16 @@ func ReadConfig(path string, fileReader codeowners.FileReader) (*Config, error)
 		SelfApprovalViaTeams:        false,
 		DisableSmartDismissal:       false,
 		RequireBothBranchReviewers:  false,
+		SuppressUnownedWarning:      false,
+		AllowSelfApproval:           false,
 		DisableReviewStatusComments: false,
 	}
+}
+
+func ReadConfig(path string, fileReader codeowners.FileReader) (*Config, error) {
+	if !strings.HasSuffix(path, "/") {
+		path += "/"
+	}
 
 	// Use filesystem reader if none provided
 	if fileReader == nil {
@@ -62,22 +66,15 @@ func ReadConfig(path string, fileReader codeowners.FileReader) (*Config, error)
 	fileName := path + "codeowners.toml"
 
 	if !fileReader.PathExists(fileName) {
-		return defaultConfig, nil
+		return newDefaultConfig(), nil
 	}
 	file, err := fileReader.ReadFile(fileName)
 	if err != nil {
-		return defaultConfig, err
-	}
-	config := defaultConfig
-	err = toml.Unmarshal(file, &config)
-	if err != nil {
-		return defaultConfig, err
-	}
-	if config.Enforcement == nil {
-		config.Enforcement = defaultConfig.Enforcement
+		return newDefaultConfig(), err
 	}
-	if config.AdminBypass == nil {
-		config.AdminBypass = defaultConfig.AdminBypass
+	config := newDefaultConfig()
+	if err := toml.Unmarshal(file, config); err != nil {
+		return newDefaultConfig(), err
 	}
 	return config, nil
 }

internal/config/config_test.go

@@ -1,8 +1,11 @@
 package owners
 
 import (
+	"fmt"
 	"os"
 	"path/filepath"
+	"reflect"
+	"strings"
 	"testing"
 )
 
@@ -250,6 +253,128 @@ func TestReadConfigFileError(t *testing.T) {
 	}
 }
 
+func TestReadConfigInvalidTomlReturnsDefaults(t *testing.T) {
+	testDir := t.TempDir()
+	content := `
+max_reviews = 9
+min_reviews = 9
+unskippable_reviewers = ["@someone"]
+ignore = ["vendor"]
+high_priority_labels = ["urgent"]
+detailed_reviewers = true
+disable_smart_dismissal = true
+require_both_branch_reviewers = true
+suppress_unowned_warning = true
+allow_self_approval = true
+self_approval_via_teams = true
+disable_review_status_comments = true
+[enforcement]
+approval = true
+fail_check = false
+[admin_bypass]
+enabled = true
+allowed_users = ["someone"]
+trailing = invalid
+`
+	if err := os.WriteFile(filepath.Join(testDir, "codeowners.toml"), []byte(content), 0644); err != nil {
+		t.Fatalf("failed to write test config: %v", err)
+	}
+
+	config, err := ReadConfig(testDir, nil)
+	if err == nil {
+		t.Fatal("expected a parse error")
+	}
+	if config == nil {
+		t.Fatal("expected a config alongside the error")
+	}
+
+	if !reflect.DeepEqual(config, newDefaultConfig()) {
+		t.Errorf("expected pristine defaults after a failed parse, got %s", configDiff(config, newDefaultConfig()))
+	}
+}
+
+func TestReadConfigNeverReturnsNilOnError(t *testing.T) {
+	testDir := t.TempDir()
+	unreadable := filepath.Join(testDir, "unreadable")
+	if err := os.Mkdir(unreadable, 0o000); err != nil {
+		t.Fatalf("mkdir: %v", err)
+	}
+	t.Cleanup(func() { _ = os.Chmod(unreadable, 0o755) })
+
+	malformed := t.TempDir()
+	if err := os.WriteFile(filepath.Join(malformed, "codeowners.toml"), []byte("trailing = invalid\n"), 0644); err != nil {
+		t.Fatalf("write: %v", err)
+	}
+
+	for _, dir := range []string{unreadable, malformed} {
+		config, err := ReadConfig(dir, nil)
+		if err == nil {
+			continue
+		}
+		if config == nil {
+			t.Fatalf("%s: callers warn and then dereference the config, so an error path must still return one", dir)
+		}
+		if !reflect.DeepEqual(config, newDefaultConfig()) {
+			t.Errorf("%s: expected pristine defaults, got %s", dir, configDiff(config, newDefaultConfig()))
+		}
+	}
+}
+
+func configDiff(got, want *Config) string {
+	problems := make([]string, 0, 8)
+	add := func(format string, args ...any) {
+		problems = append(problems, fmt.Sprintf(format, args...))
+	}
+	if got.MaxReviews != nil {
+		add("MaxReviews=%d (want nil)", *got.MaxReviews)
+	}
+	if got.MinReviews != nil {
+		add("MinReviews=%d (want nil)", *got.MinReviews)
+	}
+	if !sliceEqual(got.UnskippableReviewers, want.UnskippableReviewers) {
+		add("UnskippableReviewers=%v", got.UnskippableReviewers)
+	}
+	if !sliceEqual(got.Ignore, want.Ignore) {
+		add("Ignore=%v", got.Ignore)
+	}
+	if !sliceEqual(got.HighPriorityLabels, want.HighPriorityLabels) {
+		add("HighPriorityLabels=%v", got.HighPriorityLabels)
+	}
+	if got.Enforcement == nil {
+		add("Enforcement=nil")
+	} else if *got.Enforcement != *want.Enforcement {
+		add("Enforcement=%+v (want %+v)", *got.Enforcement, *want.Enforcement)
+	}
+	if got.AdminBypass == nil {
+		add("AdminBypass=nil")
+	} else {
+		if got.AdminBypass.Enabled != want.AdminBypass.Enabled {
+			add("AdminBypass.Enabled=%v", got.AdminBypass.Enabled)
+		}
+		if !sliceEqual(got.AdminBypass.AllowedUsers, want.AdminBypass.AllowedUsers) {
+			add("AdminBypass.AllowedUsers=%v", got.AdminBypass.AllowedUsers)
+		}
+	}
+	for _, f := range []struct {
+		name string
+		got  bool
+		want bool
+	}{
+		{"DetailedReviewers", got.DetailedReviewers, want.DetailedReviewers},
+		{"DisableSmartDismissal", got.DisableSmartDismissal, want.DisableSmartDismissal},
+		{"RequireBothBranchReviewers", got.RequireBothBranchReviewers, want.RequireBothBranchReviewers},
+		{"SuppressUnownedWarning", got.SuppressUnownedWarning, want.SuppressUnownedWarning},
+		{"AllowSelfApproval", got.AllowSelfApproval, want.AllowSelfApproval},
+		{"SelfApprovalViaTeams", got.SelfApprovalViaTeams, want.SelfApprovalViaTeams},
+		{"DisableReviewStatusComments", got.DisableReviewStatusComments, want.DisableReviewStatusComments},
+	} {
+		if f.got != f.want {
+			add("%s=%v (want %v)", f.name, f.got, f.want)
+		}
+	}
+	return strings.Join(problems, "; ")
+}
+
 // Helper functions
 func intPtr(i int) *int {
 	return &i

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,
 			},
 		},