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

ReadConfig handed the same default Config to the TOML parser and to its own
error paths. go-toml dereferences a non-nil pointer in place rather than
allocating, so a file which failed halfway left its partially parsed values in
the instance the error path then returned.

The caller logs "using default config" and carries on, with whatever the parser
managed to read before it failed. A malformed file could therefore turn
enforcement off, or widen admin bypass, while the logs said defaults were in
force.

Build a fresh instance per call instead: the parser gets its own, and every
error path builds another. The nil-section fixups go with it, since defaults can
no longer be clobbered and TOML cannot express a null table.

The regression test asserts the whole struct against pristine defaults rather
than sampling a couple of sections. On the unfixed code fourteen fields survive
the failed parse, including enforcement.approval and admin_bypass.enabled.

Coverage badge regenerated.

Author: asyncawaitpromise

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{},
@@ -53,6 +49,12 @@ func ReadConfig(path string, fileReader codeowners.FileReader) (*Config, error)
 		RequireBothBranchReviewers:  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 +64,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,10 @@
 package owners
 
 import (
+	"fmt"
 	"os"
 	"path/filepath"
+	"strings"
 	"testing"
 )
 
@@ -250,6 +252,101 @@ 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 diff := configDiff(config, newDefaultConfig()); diff != "" {
+		t.Errorf("expected pristine defaults after a failed parse, got %s", diff)
+	}
+}
+
+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,
 			},
 		},