From 81bf7a694a89da7a6371f01cc11dce0659b3f906 Mon Sep 17 00:00:00 2001 From: Ezekiel Lopez Date: Tue, 1 Sep 2026 00:07:19 -0700 Subject: [PATCH 1/2] 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. --- README.md | 2 +- internal/config/config.go | 31 +++++------ internal/config/config_test.go | 97 ++++++++++++++++++++++++++++++++++ internal/git/diff_test.go | 2 +- 4 files changed, 112 insertions(+), 20 deletions(-) diff --git a/README.md b/README.md index 90857ad..312f015 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-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) diff --git a/internal/config/config.go b/internal/config/config.go index c3bcf7e..6c3b054 100644 --- a/internal/config/config.go +++ b/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 } diff --git a/internal/config/config_test.go b/internal/config/config_test.go index f09c117..76337a9 100644 --- a/internal/config/config_test.go +++ b/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 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, }, }, From 8f106ba1fbd916baa6e8ea6cf4ee063d162d70eb Mon Sep 17 00:00:00 2001 From: Ezekiel Lopez Date: Tue, 1 Sep 2026 13:52:04 -0700 Subject: [PATCH 2/2] test: gate the malformed-config check on the whole struct Review catch. configDiff compares every field the struct has today, but it is a hand-maintained list with no link to the struct, so a field added later is a field the test silently stops covering. The open orphaned-approval PR adds one, and with it present a malformed file could leak fetch_orphaned_approval = true through the error path while the test still went green. Gate on reflect.DeepEqual against a pristine default instead, and keep configDiff only to build the failure message. A new field is then covered the moment it exists. Also enforce the contract callers already rely on: they log a warning and immediately dereference the config, so every error path has to return one. That was true but only by inspection, and is now a test. newDefaultConfig spells out every bool rather than five of seven, since it is the canonical statement of the defaults. --- internal/config/config.go | 2 ++ internal/config/config_test.go | 32 ++++++++++++++++++++++++++++++-- 2 files changed, 32 insertions(+), 2 deletions(-) diff --git a/internal/config/config.go b/internal/config/config.go index 6c3b054..c753e54 100644 --- a/internal/config/config.go +++ b/internal/config/config.go @@ -47,6 +47,8 @@ func newDefaultConfig() *Config { SelfApprovalViaTeams: false, DisableSmartDismissal: false, RequireBothBranchReviewers: false, + SuppressUnownedWarning: false, + AllowSelfApproval: false, DisableReviewStatusComments: false, } } diff --git a/internal/config/config_test.go b/internal/config/config_test.go index 76337a9..22385f8 100644 --- a/internal/config/config_test.go +++ b/internal/config/config_test.go @@ -4,6 +4,7 @@ import ( "fmt" "os" "path/filepath" + "reflect" "strings" "testing" ) @@ -287,8 +288,35 @@ trailing = invalid 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) + 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())) + } } }